Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mono.Android] abstract-ify inconsistent android.webkit API. #1304

Conversation

atsushieno
Copy link
Contributor

As part of discussion at #1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.

@atsushieno
Copy link
Contributor Author

This needs changes in xamarin-android-api-compatibility too.

@atsushieno atsushieno force-pushed the abstractify-inconsistent-webkit-api branch 2 times, most recently from 256b47c to a6faa34 Compare February 14, 2018 14:12
atsushieno added a commit to atsushieno/xamarin-android-api-compatibility that referenced this pull request Feb 14, 2018
As part of discussion at dotnet/android#1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.

(copying the commitmsg from dotnet/android#1304)
@atsushieno atsushieno force-pushed the abstractify-inconsistent-webkit-api branch from a6faa34 to bb9d123 Compare February 15, 2018 00:29
atsushieno added a commit to atsushieno/xamarin-android-api-compatibility that referenced this pull request Feb 15, 2018
As part of discussion at dotnet/android#1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.

(copying the commitmsg from dotnet/android#1304)
@atsushieno
Copy link
Contributor Author

build

@atsushieno atsushieno force-pushed the abstractify-inconsistent-webkit-api branch from bb9d123 to 3082edb Compare February 15, 2018 14:22
atsushieno added a commit to atsushieno/xamarin-android-api-compatibility that referenced this pull request Feb 16, 2018
As part of discussion at dotnet/android#1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.

(copying the commitmsg from dotnet/android#1304)
@jonpryor
Copy link
Member

#1303 (comment)

As part of discussion at dotnet#1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.
@atsushieno atsushieno force-pushed the abstractify-inconsistent-webkit-api branch from 3082edb to 2eac2a5 Compare February 22, 2018 07:50
atsushieno added a commit to atsushieno/xamarin-android-api-compatibility that referenced this pull request Feb 22, 2018
As part of discussion at dotnet/android#1078 (comment) ,
we decided to make changes to those inconsistent methods that used to be
virtual and then became abstract, to become abstract even in old API level.

It is done at metadata fixup level.

(copying the commitmsg from dotnet/android#1304)
@jonpryor
Copy link
Member

I think these changes are too aggressive. The listed changes:

<h2>Namespace Android.Webkit</h2>
<!-- start type WebHistoryItem --> <div>
<h3>Type Changed: Android.Webkit.WebHistoryItem</h3>
<p>Modified properties:</p>
<pre>
<div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> int Id { get; }
</div></pre>

This turns WebHistoryItem.getId() into an abstract member. However:

  1. WebHistoryItem no longer documents any getId() method, and
  2. The last API level to declare a WebHistoryItem.getId() method -- API-16 -- didn't declare it as abstract either.

So why was this method changed at all?

<h3>Type Changed: Android.Webkit.WebSettings</h3>
<p>Modified properties:</p>
<pre>
<div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> bool NavDump { get; set; }
</div><div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> bool PluginsEnabled { get; set; }
</div><div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> string PluginsPath { get; set; }
</div><div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> bool UseDoubleTree { get; set; }
</div><div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> bool UseWebViewBackgroundForOverscrollBackground { get; set; }
</div><div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> int UserAgent { get; set; }
</div></pre>
<p>Modified methods:</p>
<pre>
<div data-is-breaking>	public <span class='added added-breaking-inline'>abstract</span> WebSettings.TextSize GetTextSize ()
</div></pre>

These changes are in the same boat: WebSettings does not declare any of the methods getNavDump(), getPluginsEnabled(), getPluginsPath(), getUseDoubleTree(), getUseWebViewBackgroundForOverscrollBackground(), or getUserAgent(). These were apparently removed in API-17

In API-16 -- the last API level they appeared in -- none of those methods are abstract.

Thus, why are we turning these deprecated and no longer public members into abstract members?

@jonpryor
Copy link
Member

Regarding our previous discussion:

I'd suggest to change those "virtual in the past, now abstract" members to abstract AT ALL, using metadata fixup

My interpretation of this was that if a member was virtual in API-X, and became abstract in API-(X+1), then we should change this member to be abstract in all API levels.

I agree with my interpretation. For example, consider CookieManager.hasCookies(): this was non-abstract in API-1, and became abstract in API-22. By this logic, we should make the CookieManager.HasCookies property abstract in all API levels, and I'd agree with that.

This PR does not fit the above description, not in my mind. This PR is turning methods which were always non-abstract members into abstract members -- a sure fire API break, as nobody will have ever overridden these members before! -- and, as a double whammy, is turning methods which no longer exist into abstract methods.

As such, this PR would make our binding worse: anybody deriving from WebSettings will need to override and implement more members than a Java developer would need to do.

@atsushieno
Copy link
Contributor Author

huh, I am confused. With the latest run-api-compatibility-tests they are indeed not reported as inter-api breakage anymore. Then there is no reason to make API changes (I don't want to dare to deal with it either).

@atsushieno atsushieno closed this Feb 26, 2018
@jonpryor
Copy link
Member

The probable reason that run-api-compatibility-tests doesn't flag them as problematic is because it uses mono-api-html --ignore-changes-virtual, which ignores changes from virtual to abstract.

@dellis1972 dellis1972 mentioned this pull request May 17, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants