Bug 136076 - Expose injected bundle SPI to get a node's URL element, get the visible selection range of that element, and snapshot that range
Summary: Expose injected bundle SPI to get a node's URL element, get the visible selec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 136061
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-19 10:42 PDT by Peyton Randolph
Modified: 2014-08-19 16:33 PDT (History)
2 users (show)

See Also:


Attachments
Patch (17.38 KB, patch)
2014-08-19 10:48 PDT, Peyton Randolph
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2014-08-19 11:11 PDT, Peyton Randolph
no flags Details | Formatted Diff | Diff
Patch (16.98 KB, patch)
2014-08-19 14:06 PDT, Peyton Randolph
no flags Details | Formatted Diff | Diff
Patch (17.61 KB, patch)
2014-08-19 15:53 PDT, Peyton Randolph
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peyton Randolph 2014-08-19 10:42:55 PDT
Expose injected bundle SPI to get a node's URL element, get the visible selection range of that element, and snapshot that range
Comment 1 Peyton Randolph 2014-08-19 10:48:01 PDT
Created attachment 236814 [details]
Patch
Comment 2 mitz 2014-08-19 11:08:38 PDT
Comment on attachment 236814 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:79
> +WKBundleNodeHandleRef WKBundleNodeHandleGetURLElementHandle(WKBundleNodeHandleRef elementHandle);
> +

What’s this? I think you left it in here by mistake.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleRangeHandle.cpp:36
> +using namespace WebCore;

We’ve moved away from using namespace WebCore in WebKit2. Just namespace-qualify what you use below.

> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleRangeHandle.cpp:136
> +    PaintBehavior cachedPaintBehavior = frameView->paintBehavior();

We normally call something like this “oldPaintBehavior” or “savedPaintBehavior”.
Comment 3 Peyton Randolph 2014-08-19 11:11:50 PDT
Created attachment 236817 [details]
Patch
Comment 4 Peyton Randolph 2014-08-19 11:12:13 PDT
(In reply to comment #2)
> (From update of attachment 236814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236814&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNodeHandle.cpp:79
> > +WKBundleNodeHandleRef WKBundleNodeHandleGetURLElementHandle(WKBundleNodeHandleRef elementHandle);
> > +
> 
> What’s this? I think you left it in here by mistake.

You're right. Removed.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleRangeHandle.cpp:36
> > +using namespace WebCore;
> 
> We’ve moved away from using namespace WebCore in WebKit2. Just namespace-qualify what you use below.

Removed.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleRangeHandle.cpp:136
> > +    PaintBehavior cachedPaintBehavior = frameView->paintBehavior();
> 
> We normally call something like this “oldPaintBehavior” or “savedPaintBehavior”.

Changed.
Comment 5 Tim Horton 2014-08-19 11:54:13 PDT
Comment on attachment 236817 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:48
> +    PassRefPtr<InjectedBundleNodeHandle> URLElementHandle() const;

https://www.webkit.org/coding/coding-style.html#names-basic says that URL should be lowercase in all of these instances.
Comment 6 Peyton Randolph 2014-08-19 14:06:29 PDT
Created attachment 236824 [details]
Patch
Comment 7 Peyton Randolph 2014-08-19 14:06:50 PDT
(In reply to comment #5)
> (From update of attachment 236817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236817&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:48
> > +    PassRefPtr<InjectedBundleNodeHandle> URLElementHandle() const;
> 
> https://www.webkit.org/coding/coding-style.html#names-basic says that URL should be lowercase in all of these instances.

Fixed.
Comment 8 Tim Horton 2014-08-19 14:17:31 PDT
Comment on attachment 236824 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleRangeHandle.cpp:119
> +    float scaleFactor = (options & SnapshotOptionsExcludeDeviceScaleFactor) ? 1 : frame->page()->deviceScaleFactor();

It would be nice if we could share this middle block of code with InjectedBundleNodeHandle (and WebPage, etc.), but I'm not sure where to put it.
Comment 9 WebKit Commit Bot 2014-08-19 14:52:32 PDT
Comment on attachment 236824 [details]
Patch

Rejecting attachment 236824 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0AnchorTypeE", referenced from:
      __ZNK6WebKit24InjectedBundleNodeHandle12visibleRangeEv in InjectedBundleNodeHandle.o
      __ZN7WebCore18lastPositionInNodeEPNS_4NodeE in InjectedBundleNodeHandle.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** BUILD FAILED **


The following build commands failed:
	Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit normal x86_64
(1 failure)

Full output: http://webkit-queues.appspot.com/results/4618171210268672
Comment 10 Peyton Randolph 2014-08-19 15:53:25 PDT
Created attachment 236829 [details]
Patch
Comment 11 WebKit Commit Bot 2014-08-19 16:33:46 PDT
Comment on attachment 236829 [details]
Patch

Clearing flags on attachment: 236829

Committed r172780: <http://trac.webkit.org/changeset/172780>
Comment 12 WebKit Commit Bot 2014-08-19 16:33:50 PDT
All reviewed patches have been landed.  Closing bug.