Bug 85392 - Fix usage of layout types in platform code
Summary: Fix usage of layout types in platform code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2012-05-02 11:18 PDT by Emil A Eklund
Modified: 2012-05-02 13:29 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.97 KB, patch)
2012-05-02 11:25 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (8.00 KB, patch)
2012-05-02 12:45 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-05-02 11:18:21 PDT
Fix usage of layout types in platform code
Comment 1 Emil A Eklund 2012-05-02 11:25:25 PDT
Created attachment 139843 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-02 11:45:31 PDT
Comment on attachment 139843 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:552
> +    return enclosingIntRect(m_layerTransform.combined().inverse().clampedBoundsOfProjectedQuad(FloatQuad(FloatRect(m_webGraphicsLayerClient->visibleContentsRect()))));

Longest line ever. :)  I'm surprised the FloatRect() constructor is needed for this.  Maybe even the FloatQuad() one is implicit too?

> Source/WebKit/chromium/src/WebHitTestResult.cpp:50
> +    return WebPoint(roundedIntPoint(m_private->localPoint()));

Does WebPoint not implicitly construct from IntPoint?

> Source/WebKit/chromium/src/WebSurroundingText.cpp:49
> +    VisiblePosition visiblePosition(node->renderer()->positionForPoint(static_cast<IntPoint>(hitTestInfo.localPoint())));

Does .flooredPoint() not exist?  static_cast seems like an odd way to do this.
Comment 3 Emil A Eklund 2012-05-02 11:49:11 PDT
(In reply to comment #2)
> (From update of attachment 139843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139843&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:552
> > +    return enclosingIntRect(m_layerTransform.combined().inverse().clampedBoundsOfProjectedQuad(FloatQuad(FloatRect(m_webGraphicsLayerClient->visibleContentsRect()))));
> 
> Longest line ever. :)  I'm surprised the FloatRect() constructor is needed for this.  Maybe even the FloatQuad() one is implicit too?

Was needed to avoid ambiguous type errors.

> 
> > Source/WebKit/chromium/src/WebHitTestResult.cpp:50
> > +    return WebPoint(roundedIntPoint(m_private->localPoint()));
> 
> Does WebPoint not implicitly construct from IntPoint?

It should but again there might be ambiguity. This really seems like it should work without it though.


> > Source/WebKit/chromium/src/WebSurroundingText.cpp:49
> > +    VisiblePosition visiblePosition(node->renderer()->positionForPoint(static_cast<IntPoint>(hitTestInfo.localPoint())));
> 
> Does .flooredPoint() not exist?  static_cast seems like an odd way to do this.

The static_cast is not used to floor the value, it is used to convert the WebPoint to an IntPoint (as the implicit conversion gets confused by the multiple available conversion paths). I described this in the ChangeLog, let me know if you'd like me to also add a comment to that effect.
Comment 4 Emil A Eklund 2012-05-02 12:45:09 PDT
Created attachment 139861 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-05-02 13:29:00 PDT
Comment on attachment 139861 [details]
Patch for landing

Clearing flags on attachment: 139861

Committed r115877: <http://trac.webkit.org/changeset/115877>
Comment 6 WebKit Review Bot 2012-05-02 13:29:17 PDT
All reviewed patches have been landed.  Closing bug.