Bug 58823

Summary: [chromium] expose title direction to webkit client
Product: WebKit Reporter: Evan Martin <evan>
Component: New BugsAssignee: Evan Martin <evan>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, fishd
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 Evan Martin 2011-04-18 13:53:14 PDT
[chromium] expose title direction to webkit client
Comment 1 Evan Martin 2011-04-18 13:56:09 PDT
Created attachment 90085 [details]
Patch
Comment 2 Evan Martin 2011-04-18 14:01:04 PDT
Created attachment 90089 [details]
Patch
Comment 3 Evan Martin 2011-04-18 14:02:10 PDT
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 Evan Martin 2011-04-18 14:52:32 PDT
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 Evan Martin 2011-04-18 15:05:28 PDT
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 Eric Seidel 2011-04-18 15:17:33 PDT
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 Evan Martin 2011-04-18 15:49:55 PDT
Created attachment 90108 [details]
Patch
Comment 8 Evan Martin 2011-04-18 15:50:24 PDT
OK, my new patch has a fixed ChangeLog and includes the addition to Skipped lists.  I think it's ready for review.
Comment 9 Eric Seidel 2011-04-18 15:51:46 PDT
Comment on attachment 90108 [details]
Patch

Looks fine to me.
Comment 10 Evan Martin 2011-04-18 15:58:49 PDT
Committed r84199: <http://trac.webkit.org/changeset/84199>
Comment 11 Darin Fisher (:fishd, Google) 2011-04-19 10:19:25 PDT
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);
  }
Comment 12 Evan Martin 2011-04-19 11:20:12 PDT
Good catch, Darin.  I've uploaded a patch on bug 58909.