Bug 67573 - [Chromium] Add WebFloatQuad.h for Android
Summary: [Chromium] Add WebFloatQuad.h for Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2011-09-04 02:18 PDT by Adam Barth
Modified: 2011-09-06 15:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2011-09-04 02:23 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (6.99 KB, patch)
2011-09-04 13:21 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2011-09-05 11:32 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (7.17 KB, patch)
2011-09-06 14:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-09-04 02:18:00 PDT
[Chromium] Add WebFloatQuad.h for Android
Comment 1 Adam Barth 2011-09-04 02:23:37 PDT
Created attachment 106281 [details]
Patch
Comment 2 Adam Barth 2011-09-04 02:25:11 PDT
+fishd for WebKit API review.
Comment 3 Darin Fisher (:fishd, Google) 2011-09-04 08:59:14 PDT
Comment on attachment 106281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106281&action=review

> Source/WebKit/chromium/public/WebFloatQuad.h:76
> +    WebRect enclosingRect() const

this seems like a fair chunk of code that perhaps should not be inlined.
add a new WEBKIT_EXPORT method instead?

also note that we usually put all non-WEBKIT_IMPLEMENTATION public methods
above WEBKIT_IMPLEMENTATION methods.
Comment 4 Adam Barth 2011-09-04 13:21:15 PDT
Created attachment 106292 [details]
Patch
Comment 5 WebKit Review Bot 2011-09-04 13:25:07 PDT
Comment on attachment 106292 [details]
Patch

Attachment 106292 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9593287
Comment 6 Adam Barth 2011-09-04 13:29:30 PDT
Comment on attachment 106292 [details]
Patch

/me investigates.
Comment 7 WebKit Review Bot 2011-09-04 14:08:11 PDT
Comment on attachment 106292 [details]
Patch

Attachment 106292 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9594135
Comment 8 Peter Beverloo 2011-09-05 03:56:13 PDT
WebFloatPoint.h includes IntPoint.h on line 37 which declares WebCore::IntPoint, while we need WebCore::FloatPoint (FloatPoint.h). Other files including WebFloatPoint.h probably reach FloatPoint.h through other includes.
Comment 9 Adam Barth 2011-09-05 11:32:22 PDT
Created attachment 106348 [details]
Patch
Comment 10 Adam Barth 2011-09-05 11:47:17 PDT
Thanks Peter.
Comment 11 Darin Fisher (:fishd, Google) 2011-09-06 13:42:21 PDT
Comment on attachment 106348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106348&action=review

> Source/WebKit/chromium/public/WebFloatQuad.h:37
> +#include <algorithm>

nit: shouldn't system includes be listed first?

> Source/WebKit/chromium/public/WebFloatQuad.h:79
> +}

nit: } // namespace WebKit

see part 3 of the webkit style guide
Comment 12 Adam Barth 2011-09-06 14:07:35 PDT
(In reply to comment #11)
> (From update of attachment 106348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106348&action=review
> 
> > Source/WebKit/chromium/public/WebFloatQuad.h:79
> > +}
> 
> nit: } // namespace WebKit
> 
> see part 3 of the webkit style guide

Oh!  I've been removing these.  I thought we had updated the style guide to say that we should skip them, but maybe that was only for the #if/#endif header guards.
Comment 13 Adam Barth 2011-09-06 14:14:54 PDT
(In reply to comment #11)
> (From update of attachment 106348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106348&action=review
> 
> > Source/WebKit/chromium/public/WebFloatQuad.h:37
> > +#include <algorithm>
> 
> nit: shouldn't system includes be listed first?

"Includes of system headers must come after includes of other headers."

http://www.webkit.org/coding/coding-style.html
Comment 14 Adam Barth 2011-09-06 14:16:39 PDT
Created attachment 106484 [details]
Patch for landing
Comment 15 WebKit Review Bot 2011-09-06 15:18:55 PDT
Comment on attachment 106484 [details]
Patch for landing

Clearing flags on attachment: 106484

Committed r94604: <http://trac.webkit.org/changeset/94604>
Comment 16 WebKit Review Bot 2011-09-06 15:18:59 PDT
All reviewed patches have been landed.  Closing bug.