Bug 58823 - [chromium] expose title direction to webkit client
Summary: [chromium] expose title direction to webkit client
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Evan Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-18 13:53 PDT by Evan Martin
Modified: 2011-04-19 11:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.23 KB, patch)
2011-04-18 13:56 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (11.20 KB, patch)
2011-04-18 14:01 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (13.84 KB, patch)
2011-04-18 15:49 PDT, Evan Martin
fishd: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 (no email) 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 (no email) 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.