Bug 67573 – [Chromium] Add WebFloatQuad.h for Android

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

[Chromium] Add WebFloatQuad.h for Android

[Adam Barth]

Created attachment 106281 [details]
Patch

[Adam Barth]

+fishd for WebKit API review.

[Darin Fisher (:fishd, Google)]

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]

Created attachment 106292 [details]
Patch

[WebKit Review Bot]

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 on attachment 106292 [details]
Patch

/me investigates.

[WebKit Review Bot]

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]

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]

Created attachment 106348 [details]
Patch

[Adam Barth]

Thanks Peter.

[Darin Fisher (:fishd, Google)]

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]

(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]

(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]

Created attachment 106484 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 106484 [details]
Patch for landing

Clearing flags on attachment: 106484

Committed r94604: <http://trac.webkit.org/changeset/94604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.