Bug 58823 – [chromium] expose title direction to webkit client

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Evan Martin]

[chromium] expose title direction to webkit client

[Evan Martin]

Created attachment 90085 [details]
Patch

[Evan Martin]

Created attachment 90089 [details]
Patch

[Evan Martin]

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?

[Evan Martin]

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?

[Evan Martin]

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.

[Eric Seidel (no email)]

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.

[Evan Martin]

Created attachment 90108 [details]
Patch

[Evan Martin]

OK, my new patch has a fixed ChangeLog and includes the addition to Skipped lists.  I think it's ready for review.

[Eric Seidel (no email)]

Comment on attachment 90108 [details]
Patch

Looks fine to me.

[Evan Martin]

Committed r84199: <http://trac.webkit.org/changeset/84199>

[Darin Fisher (:fishd, Google)]

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);
  }

[Evan Martin]

Good catch, Darin.  I've uploaded a patch on bug 58909.