Summary: | [Chromium] Add WebFloatQuad.h for Android | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, fishd, peter, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 66687 | ||||||||||||
Attachments: |
|
Description
Adam Barth
2011-09-04 02:18:00 PDT
Created attachment 106281 [details]
Patch
+fishd for WebKit API review. 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. Created attachment 106292 [details]
Patch
Comment on attachment 106292 [details] Patch Attachment 106292 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9593287 Comment on attachment 106292 [details]
Patch
/me investigates.
Comment on attachment 106292 [details] Patch Attachment 106292 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9594135 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. Created attachment 106348 [details]
Patch
Thanks Peter. 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 (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. (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 Created attachment 106484 [details]
Patch for landing
Comment on attachment 106484 [details] Patch for landing Clearing flags on attachment: 106484 Committed r94604: <http://trac.webkit.org/changeset/94604> All reviewed patches have been landed. Closing bug. |