|
|
Created:
6 years, 7 months ago by AviD Modified:
6 years, 6 months ago Reviewers:
jschuh, jam, abarth-chromium, Inactive, juhui24.lee, eseidel CC:
abarth-chromium, blink-reviews, dglazkov+blink, jamesr Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix to remove customised String over IPC for SmartClip
Currently, the data for SmartClip is sent over IPC as a customized
string, which contains rect values and string of content within the
clip rect. Changes from Blink side to remove custom serialization and
send it as a structure of rect and string values.
BUG=330872
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173295
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176646
Patch Set 1 #
Total comments: 2
Patch Set 2 : changes done according to review comments #
Total comments: 2
Patch Set 3 : Changed as per review #Patch Set 4 : check for webviewtest #Patch Set 5 : Avoid build dependency on chromium checkin #Patch Set 6 : Added TODO in WebView.h (Avoid build dependency) #
Total comments: 4
Patch Set 7 : changed according to review comments by eseidel #
Total comments: 8
Patch Set 8 : Updated according to review comments #
Total comments: 1
Patch Set 9 : Fixed nits #Patch Set 10 : rebase to latest #Patch Set 11 : Build error #
Created: 6 years, 6 months ago
Messages
Total messages: 33 (0 generated)
PTAL From the existing code, its not required to send a vector of structure of rect and string values. Only a structure of rect and string values is sufficient.
Adding ch.dumez in reviewers list.
https://codereview.chromium.org/259263003/diff/1/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/259263003/diff/1/public/web/WebViewClient.h#n... public/web/WebViewClient.h:218: virtual void updateSmartClipDataResult(const WebString& text, const WebRect& rect) { } Why not just use an out parameters on the WebView function?
Changes made as per review comments. PTAL https://codereview.chromium.org/259263003/diff/1/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/259263003/diff/1/public/web/WebViewClient.h#n... public/web/WebViewClient.h:218: virtual void updateSmartClipDataResult(const WebString& text, const WebRect& rect) { } On 2014/04/30 16:59:43, abarth wrote: > Why not just use an out parameters on the WebView function? Done.
lgtm https://codereview.chromium.org/259263003/diff/20001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/259263003/diff/20001/public/web/WebView.h#new... public/web/WebView.h:421: virtual void getSmartClipData(WebRect) = 0; virtual void getSmartClipData(WebRect, WebString*, WebRect*) OVERRIDE; These signatures don't quite match... https://codereview.chromium.org/259263003/diff/20001/public/web/WebView.h#new... public/web/WebView.h:422: Two blank lines between sections, pls
Thanks abarth. Not sure how that was missed. PTAL
relgtm
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/60001
Message was sent while issue was closed.
Change committed as 173295
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/266193003/ by loislo@chromium.org. The reason for reverting is: android build was broken by this patch http://build.chromium.org/p/chromium.webkit/builders/Android%20Builder/builds....
@abarth I have removed the build dependency on https://codereview.chromium.org/260623004/ Could you PTAL?
On 2014/06/02 11:30:42, AviD wrote: > @abarth > I have removed the build dependency on > https://codereview.chromium.org/260623004/ > > Could you PTAL? I believe abarth@ is still away. You will likely want to find another OWNER to take a look.
https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... File Source/core/frame/SmartClip.h (right): https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... Source/core/frame/SmartClip.h:52: IntRect getRect() const; We don't use "get" for getters. Does this rect have a more descriptive name? I guess smartClip.rect() is fine. https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... Source/core/frame/SmartClip.h:53: String getClipData() const; How is this different from the toString above?
@eseidel updated according to review comments. Could you PTAL? https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... File Source/core/frame/SmartClip.h (right): https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... Source/core/frame/SmartClip.h:52: IntRect getRect() const; On 2014/06/02 16:37:50, eseidel wrote: > We don't use "get" for getters. Does this rect have a more descriptive name? I > guess smartClip.rect() is fine. Done. https://codereview.chromium.org/259263003/diff/100001/Source/core/frame/Smart... Source/core/frame/SmartClip.h:53: String getClipData() const; On 2014/06/02 16:37:50, eseidel wrote: > How is this different from the toString above? the toString() had customized string with both rect and string values. (crbug.com/330872) getClipData() only retruns the m_string value so that it can be sent separately over IPC, avoiding pickling of data. Due to custom serialization, SmartClip feature works only in single process mode. (https://codereview.chromium.org/260623004/diff/20001/content/browser/renderer...) With removal of custom serialization, this feature can work in multi process mode too.
https://codereview.chromium.org/259263003/diff/120001/Source/core/frame/Smart... File Source/core/frame/SmartClip.cpp (right): https://codereview.chromium.org/259263003/diff/120001/Source/core/frame/Smart... Source/core/frame/SmartClip.cpp:69: String SmartClipData::clipData() const Could return a const String&. https://codereview.chromium.org/259263003/diff/120001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/259263003/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:249: // TODO(AviD):This should be removed when the chromium side patch lands Please use FIXME and without attribution: http://dev.chromium.org/blink/coding-style#TOC-Comments Also, missing space after ':'
https://codereview.chromium.org/259263003/diff/120001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/259263003/diff/120001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:1499: EXPECT_FALSE(clipData.isEmpty());} What's up with this trailing '}'? https://codereview.chromium.org/259263003/diff/120001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/259263003/diff/120001/public/web/WebView.h#ne... public/web/WebView.h:412: // TODO(AviD):This should be removed when the chromium side patch lands Same comment about FIXME without attribution.
@Chris Dumez @abarth: Updated according to review comments. Could you PTAL? https://codereview.chromium.org/259263003/diff/120001/Source/core/frame/Smart... File Source/core/frame/SmartClip.cpp (right): https://codereview.chromium.org/259263003/diff/120001/Source/core/frame/Smart... Source/core/frame/SmartClip.cpp:69: String SmartClipData::clipData() const On 2014/06/09 14:00:51, Chris Dumez wrote: > Could return a const String&. Done. https://codereview.chromium.org/259263003/diff/120001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/259263003/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:249: // TODO(AviD):This should be removed when the chromium side patch lands On 2014/06/09 14:00:51, Chris Dumez wrote: > Please use FIXME and without attribution: > http://dev.chromium.org/blink/coding-style#TOC-Comments > > Also, missing space after ':' Done. https://codereview.chromium.org/259263003/diff/120001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/259263003/diff/120001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:1499: EXPECT_FALSE(clipData.isEmpty());} On 2014/06/09 14:47:10, Chris Dumez wrote: > What's up with this trailing '}'? Done. https://codereview.chromium.org/259263003/diff/120001/public/web/WebView.h File public/web/WebView.h (right): https://codereview.chromium.org/259263003/diff/120001/public/web/WebView.h#ne... public/web/WebView.h:412: // TODO(AviD):This should be removed when the chromium side patch lands On 2014/06/09 14:47:10, Chris Dumez wrote: > Same comment about FIXME without attribution. Done.
On 2014/06/13 11:52:48, AviD wrote: > @Chris Dumez @abarth: Updated according to review comments. > > Could you PTAL? Thanks. Adam should probably take a look as he initially reviewed this and most changes are in web/.
LGTM w/ nit below https://codereview.chromium.org/259263003/diff/140001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/259263003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:252: virtual void getSmartClipData(WebRect, WebString*, WebRect*) OVERRIDE; WebString* -> WebString& WebRect* -> WebRect& See getSelectionRootBounds for another example of using & for out-params.
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/frame/SmartClip.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file Source/core/frame/SmartClip.cpp Hunk #1 FAILED at 61. 1 out of 1 hunk FAILED -- saving rejects to file Source/core/frame/SmartClip.cpp.rej Patch: Source/core/frame/SmartClip.cpp Index: Source/core/frame/SmartClip.cpp diff --git a/Source/core/frame/SmartClip.cpp b/Source/core/frame/SmartClip.cpp index 3a95095c06e4ef057ba19fe6b31f9962b2223556..b8b53685a2892e41e0a3407f5812b80d24439346 100644 --- a/Source/core/frame/SmartClip.cpp +++ b/Source/core/frame/SmartClip.cpp @@ -61,33 +61,14 @@ static Node* nodeInsideFrame(Node* node) return 0; } -// FIXME: SmartClipData is eventually returned via -// SLookSmartClip.DataExtractionListener: -// http://img-developer.samsung.com/onlinedocs/sms/com/samsung/android/sdk/look/... -// however the original author of this change chose to use a string-serialization -// format (presumably to make IPC easy?). -// If we're going to use this as a Pickle format, we should at least have the -// read/write code in one place! -String SmartClipData::toString() +IntRect SmartClipData::rect() const { - if (!m_node) - return emptyString(); - - const UChar fieldSeparator = 0xFFFE; - const UChar rowSeparator = 0xFFFF; + return m_rect; +} - StringBuilder result; - result.append(String::number(m_rect.x())); - result.append(fieldSeparator); - result.append(String::number(m_rect.y())); - result.append(fieldSeparator); - result.append(String::number(m_rect.width())); - result.append(fieldSeparator); - result.append(String::number(m_rect.height())); - result.append(fieldSeparator); - result.append(m_string); - result.append(rowSeparator); - return result.toString(); +const String& SmartClipData::clipData() const +{ + return m_string; } SmartClip::SmartClip(PassRefPtr<LocalFrame> frame)
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/170001
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/190001
Message was sent while issue was closed.
Change committed as 176646 |