Bug 58823

Summary: [chromium] expose title direction to webkit client
Product: WebKit Reporter: Evan Martin <evan@chromium.org>
Component: New BugsAssignee: Evan Martin <evan@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: eric@webkit.org, fishd@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Mac OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch fishd: review-

Description From 2011-04-18 13:53:14 PST
[chromium] expose title direction to webkit client
------- Comment #1 From 2011-04-18 13:56:09 PST -------
Created an attachment (id=90085) [details]
Patch
------- Comment #2 From 2011-04-18 14:01:04 PST -------
Created an attachment (id=90089) [details]
Patch
------- Comment #3 From 2011-04-18 14:02:10 PST -------
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?
------- Comment #4 From 2011-04-18 14:52:32 PST -------
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?
------- Comment #5 From 2011-04-18 15:05:28 PST -------
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.
------- Comment #6 From 2011-04-18 15:17:33 PST -------
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.
------- Comment #7 From 2011-04-18 15:49:55 PST -------
Created an attachment (id=90108) [details]
Patch
------- Comment #8 From 2011-04-18 15:50:24 PST -------
OK, my new patch has a fixed ChangeLog and includes the addition to Skipped lists.  I think it's ready for review.
------- Comment #9 From 2011-04-18 15:51:46 PST -------
(From update of attachment 90108 [details])
Looks fine to me.
------- Comment #10 From 2011-04-18 15:58:49 PST -------
Committed r84199: <http://trac.webkit.org/changeset/84199>
------- Comment #11 From 2011-04-19 10:19:25 PST -------
(From update of attachment 90108 [details])
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);
  }
------- Comment #12 From 2011-04-19 11:20:12 PST -------
Good catch, Darin.  I've uploaded a patch on bug 58909.