Summary: | Allow embedder to observe changes to frame names | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fady Samuel <fsamuel> | ||||||
Component: | New Bugs | Assignee: | Fady Samuel <fsamuel> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, ap, dglazkov, fishd, jamesr, japhet, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Fady Samuel
2012-12-07 14:44:13 PST
Created attachment 178283 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Could you please elaborate on what a NOT FOR REVIEW bug with a patch marked for review means? I don't understand how it's helpful to have "NOT FOR REVIEW" in bug title in any circumstances, but having a patch for review makes it even stranger. @Fady - if you're using "webkit-patch upload" use the "--no-review" flag to upload (In reply to comment #4) > @Fady - if you're using "webkit-patch upload" use the "--no-review" flag to upload Yup, I remembered that after I posted the patch :-) Sorry for the confusion. Thanks. This change is still in discussion. If we decide this is useful, I request a review. Thanks. (In reply to comment #5) > (In reply to comment #4) > > @Fady - if you're using "webkit-patch upload" use the "--no-review" flag to upload > > Yup, I remembered that after I posted the patch :-) Sorry for the confusion. Thanks. This change is still in discussion. If we decide this is useful, I request a review. Thanks. I'd like to go ahead and get a review for this, please. Comment on attachment 178283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178283&action=review > Source/WebCore/page/ChromeClient.h:148 > + virtual void frameNameChanged(Frame*, const String&) { } this feels like something that should be plumbed through the FrameLoaderClient > Source/WebKit/chromium/public/WebViewClient.h:165 > + virtual void frameNameChanged(WebFrame*, const WebString&) { } nit: we usually go with a format more like didChangeFoo or willChangeFoo. it seems like the consistent approach would be didChangeFrameName, but how about putting this on WebFrameClient instead? Then, it can just be didChangeName as a WebFrame already has a name attribute. Created attachment 178945 [details]
Patch
Comment on attachment 178283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178283&action=review >> Source/WebCore/page/ChromeClient.h:148 >> + virtual void frameNameChanged(Frame*, const String&) { } > > this feels like something that should be plumbed through the FrameLoaderClient Done. >> Source/WebKit/chromium/public/WebViewClient.h:165 >> + virtual void frameNameChanged(WebFrame*, const WebString&) { } > > nit: we usually go with a format more like didChangeFoo or willChangeFoo. > it seems like the consistent approach would be didChangeFrameName, but how > about putting this on WebFrameClient instead? Then, it can just be didChangeName > as a WebFrame already has a name attribute. Done. Comment on attachment 178945 [details] Patch Clearing flags on attachment: 178945 Committed r137776: <http://trac.webkit.org/changeset/137776> All reviewed patches have been landed. Closing bug. |