RESOLVED FIXED 67573
[Chromium] Add WebFloatQuad.h for Android 
https://bugs.webkit.org/show_bug.cgi?id=67573
Summary [Chromium] Add WebFloatQuad.h for Android
Adam Barth
Reported 2011-09-04 02:18:00 PDT
[Chromium] Add WebFloatQuad.h for Android
Attachments
Patch (4.57 KB, patch)
2011-09-04 02:23 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (6.99 KB, patch)
2011-09-04 13:21 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (7.43 KB, patch)
2011-09-05 11:32 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.17 KB, patch)
2011-09-06 14:16 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2011-09-04 02:23:37 PDT
Created attachment 106281 [details] Patch
Adam Barth
Comment 2 2011-09-04 02:25:11 PDT
+fishd for WebKit API review.
Darin Fisher (:fishd, Google)
Comment 3 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.
Adam Barth
Comment 4 2011-09-04 13:21:15 PDT
Created attachment 106292 [details] Patch
WebKit Review Bot
Comment 5 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
Adam Barth
Comment 6 2011-09-04 13:29:30 PDT
Comment on attachment 106292 [details] Patch /me investigates.
WebKit Review Bot
Comment 7 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
Peter Beverloo
Comment 8 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.
Adam Barth
Comment 9 2011-09-05 11:32:22 PDT
Created attachment 106348 [details] Patch
Adam Barth
Comment 10 2011-09-05 11:47:17 PDT
Thanks Peter.
Darin Fisher (:fishd, Google)
Comment 11 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
Adam Barth
Comment 12 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.
Adam Barth
Comment 13 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
Adam Barth
Comment 14 2011-09-06 14:16:39 PDT
Created attachment 106484 [details] Patch for landing
WebKit Review Bot
Comment 15 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>
WebKit Review Bot
Comment 16 2011-09-06 15:18:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.