Summary: | [chromium] expose title direction to webkit client | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Martin <evan> | ||||||||
Component: | New Bugs | Assignee: | Evan Martin <evan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, fishd | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Evan Martin
2011-04-18 13:53:14 PDT
Created attachment 90085 [details]
Patch
Created attachment 90089 [details]
Patch
I am modifying the Chromium API here, so I think Darin needs to review. Advice on how to not break Mac is appreciated; nobody other than Chromium will pass these tests until layoutTestController is changed for them, but I think that will require modifying the other ports' webkit API. Should I do that? I chatted with some people on IRC. My options are: 1) adjust public WebKit APIs for other ports like I did here. This, I learned, is a no-no. 2) add private WebKit APIs for these functions for other ports 3) check in x-fail results for this test 4) mark the test skipped I think I'd like to do #4 if possible; there's little point in adding a private API to test this code, since the whole point of this change is to add public API for the browser to get at this field in the normal title callback. Another way of writing this: it'd be trivial to write a test that got at this field using normal web APIs, just call getComputedStyle(); the purpose of the change is to expose that value in the public API. Maybe a layout test is the wrong place to test API-only changes? Ojan suggested option (5): no test in WebKit at all. Since the purpose of this change is to expose API, perhaps it is better tested by the API consumer. That would reduce this patch to a five-liner. Yup. I wouldn't worry about teh other ports. Just fix chromium, add the tests, and skip the tests on all othe rports. Ideally filing bugs against them to fix the test for their port. Created attachment 90108 [details]
Patch
OK, my new patch has a fixed ChangeLog and includes the addition to Skipped lists. I think it's ready for review. Comment on attachment 90108 [details]
Patch
Looks fine to me.
Committed r84199: <http://trac.webkit.org/changeset/84199> Comment on attachment 90108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90108&action=review > Source/WebKit/chromium/public/WebFrameClient.h:211 > + virtual void didReceiveTitle(WebFrame*, const WebString& title, WebTextDirection direction = WebTextDirectionDefault) { } how does this not break chromium? since chromium (RenderView) implements WebFrameClient, i think you probably meant to write two didReceiveTitle functions and have the newer one delegate to the older one, like this: virtual void didReceiveTitle(WebFrame*, const WebString& title) { } virtual void didReceiveTitle(WebFrame* frame, const WebString& title, WebTextDirection direction) { didReceiveTitle(frame, title); } Good catch, Darin. I've uploaded a patch on bug 58909. |