| Summary: | 1.9.90 drops symbols, breaking compatibility | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | seb128 | ||||||||
| Component: | WebKit API | Assignee: | Xan Lopez <xan.lopez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, cgarcia, mrobinson, ojan, webkit.review.bot, xan.lopez | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
seb128
2012-08-08 07:35:25 PDT
Created attachment 157433 [details]
Patch
As for the named flow issue, the property was renamed from 'overflow' to 'overset', that's why it's missing. I'd suggest to just add dummy accessors to keep API compatibility, and make them print a warning explaining the property has changed its name. I think this makes more sense because: - I bet this thing is pretty much not used at all. - Adding a proxy property that forwards to the new one would be cumbersome. - We already did this once (check WebKitDOMCustom), without any real problems. Comment on attachment 157433 [details] Patch Rejecting attachment 157433 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ONFLICT (content): Merge conflict in Source/WebKit/chromium/ChangeLog Failed to merge in the changes. Patch failed at 0001 Web Inspector: improve large array logging experience When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/13452994 Comment on attachment 157433 [details]
Patch
mmm, ok, let's try again
Comment on attachment 157433 [details] Patch Clearing flags on attachment: 157433 Committed r125183: <http://trac.webkit.org/changeset/125183> All reviewed patches have been landed. Closing bug. (In reply to comment #2) > As for the named flow issue, the property was renamed from 'overflow' to 'overset', that's why it's missing. I'd suggest to just add dummy accessors to keep API compatibility, and make them print a warning explaining the property has changed its name. I think this makes more sense because: > > - I bet this thing is pretty much not used at all. > - Adding a proxy property that forwards to the new one would be cumbersome. > - We already did this once (check WebKitDOMCustom), without any real problems. You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property? (In reply to comment #7) > You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property? You can, but as I said it's more complicated. You have to change the perl scripts to special case this. IMHO it's not really worth it. (In reply to comment #8) > (In reply to comment #7) > > You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property? > > You can, but as I said it's more complicated. You have to change the perl scripts to special case this. IMHO it's not really worth it. hmm, that would be an api break, I'm not sure if a removed property breaks the ABI too. If the ABI is broken, I think it's worth to properly fix it to not break it. Reopening, 1.9.90 still breaks api, it drops those: webkit_dom_element_get_webkit_region_overflow@Base 1.7.90 webkit_dom_html_element_get_class_list@Base 1.3.10 webkit_dom_webkit_named_flow_get_content_nodes@Base 1.9.2 webkit_dom_webkit_named_flow_get_regions_by_content_node@Base 1.9.2 webkit_video_sink_get_type@Base 1.3.10 webkit_video_sink_new@Base 1.3.10 the webkit_dom symbols are least used to be part of installed headers and are an api break webkit_dom_html_element_get_class_list() for example seems to be used in geary (http://redmine.yorba.org/projects/geary/wiki) it's used in pidgin as well... Is there some way we can catch these earlier? > Is there some way we can catch these earlier? If you want some details on the Debian packaging tool: http://lists.debian.org/debian-devel/2007/06/msg00197.html But basically run "nm -D libwebkit*.so > symbols.$version" for on n-1 and n (you could do it by commit granularity if you rebuild on every commit) and flag something red when T symbols are dropped one other thing which should be easy to do is to diff the include directories of both version, if definitions got dropped from a public .h you have issues... Those are all too hard. How about having the GTK bubble turn red when we screw this up? We could probably add this as a build step. Created attachment 161261 [details]
Patch
(In reply to comment #18) > Created an attachment (id=161261) [details] > Patch This should do the trick, the changes are mostly harmless renames. The only other API break I'm aware of is the one for cloneNode, but that one is trickier. Before solving it I'd like to get a reply to my question here https://bugs.webkit.org/show_bug.cgi?id=91704#c11, maybe Adam knows what's up with that. Attachment 161261 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:39: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:40: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:41: The parameter name "namedFlow" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:42: The parameter name "namedFlow" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 4 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #16) > Those are all too hard. How about having the GTK bubble turn red when we screw this up? The thing is that the tool gives a fair amount of false positives, so I'm not sure how easy it would be to completely automate this. We would probably need to keep a list of all the actual API we want to maintain, and make the bot turn red if anything in that list goes away or changes. It seems like we could limit the (In reply to comment #21) > (In reply to comment #16) > > Those are all too hard. How about having the GTK bubble turn red when we screw this up? > > The thing is that the tool gives a fair amount of false positives, so I'm not sure how easy it would be to completely automate this. We would probably need to keep a list of all the actual API we want to maintain, and make the bot turn red if anything in that list goes away or changes. Perhaps we could limit the false positives by limiting the scope of the API watcher to the DOM APIs, which shouldn't change. I imagine this could work something like the bindings tests. Created attachment 161309 [details]
Patch
Comment on attachment 161309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161309&action=review > Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:76 > +WebKitDOMDOMTokenList* > +webkit_dom_html_element_get_class_list(WebKitDOMHTMLElement* element) > +{ > + return webkit_dom_element_get_class_list(WEBKIT_DOM_ELEMENT(element)); > +} You should probably follow WebKit style conventions here and keep the method signature on one line. (In reply to comment #24) > You should probably follow WebKit style conventions here and keep the method signature on one line. Yeah, although amusingly the warning is not because of that. I'm just following the existing style, we should do a patch fixing everything. Comment on attachment 161309 [details] Patch Clearing flags on attachment: 161309 Committed r127124: <http://trac.webkit.org/changeset/127124> All reviewed patches have been landed. Closing bug. |