Bug 91656 – Previous API changes to Webframe causes failures in certain apps

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Roger Fong]

Changes to the resumeAnimations and suspendAnimations methods in WebFrame causes certain apps to crash because of inconsistencies in the COM interface.
The counterValueByElementId method was also removed.
However, for all of these methods, no method stubs were added where appropriate after removal.
Re-adding resume and suspend methods back into WebFrame API and adding unused method stub for counterValueByElementId.

resumeAnimations: http://trac.webkit.org/changeset/116729
suspendAnimations: http://trac.webkit.org/changeset/116610
counterValueByElementId: http://trac.webkit.org/changeset/120054

[Radar WebKit Bug Importer]

<rdar://problem/11904605>

[Roger Fong]

Radar: <rdar://problem/11904605>

[Roger Fong]

Created attachment 153371 [details]
Patch to re add in API changes to WebFrame that previously made dependent apps crash

I also rearranged method definitions in WebFrame.h to match the idl file...though this wasn't necessary...
I can roll those back if people want me too...

[WebKit Review Bot]

Attachment 153371 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/win/WebFrame.h:243:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/win/WebFrame.h:280:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

(In reply to comment #3)
> Created an attachment (id=153371) [details]
> Patch to re add in API changes to WebFrame that previously made dependent apps crash
> 
> I also rearranged method definitions in WebFrame.h to match the idl file...though this wasn't necessary...
> I can roll those back if people want me too...

I'm thinking that making the minimum necessary change is probably best.

[Tim Horton]

Comment on attachment 153371 [details]
Patch to re add in API changes to WebFrame that previously made dependent apps crash

View in context: https://bugs.webkit.org/attachment.cgi?id=153371&action=review

> Source/WebKit/win/ChangeLog:11
> +        Changes to the WebFrame API causes certain apps to crash because of inconsistencies in the COM interface.
> +        The resumeAnimations and suspendAnimations are removed, which are still needed.
> +        The counterValueByElementId method was also removed, although is not needed there.
> +        However, for all of these methods, no method stubs were added where appropriate after removal.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=91656
> +        Radar: <rdar://problem/11904605>
> +        
> +        Reviewed by NOBODY (OOPS!).

Format should be:

<title>
<bugzilla url>
<radar url>

<reviewed by>

<long description>

<file/change list>

(i.e. remove the "Radar:" and move the long description below the reviewed by line).

> Source/WebKit/win/ChangeLog:24
> +        Rearranged ordering of methods of method definitions to match interface file.

Ideally we'd do this in another patch (or not at all).

[Roger Fong]

Created attachment 153563 [details]
Rolled back previous cleanliness changes

[Roger Fong]

and fixed changelog

[Roger Fong]

Created attachment 153632 [details]
Forgot to get rid of "Radar: " in changelog of previous patch

[Jon Honeycutt]

Comment on attachment 153632 [details]
Forgot to get rid of "Radar: " in changelog of previous patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153632&action=review

r=me

> Source/WebKit/win/ChangeLog:3
> +        Changes to the WebFrame API causes dependent apps to crash.

Typo: cause, not causes.

> Source/WebKit/win/ChangeLog:9
> +        Changes to the WebFrame API causes certain apps to crash because of inconsistencies in the COM interface.

Same here.

[Roger Fong]

Created attachment 154468 [details]
Fixed grammar in changelog

[Jon Honeycutt]

Comment on attachment 154468 [details]
Fixed grammar in changelog

r=me

[WebKit Review Bot]

Comment on attachment 154468 [details]
Fixed grammar in changelog

Clearing flags on attachment: 154468

Committed r123688: <http://trac.webkit.org/changeset/123688>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

http://trac.webkit.org/changeset/123688