Fix to remove customised String over IPC for SmartClip

Fix 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

[AviD, 2014-04-30 11:32:05]

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.

[AviD, 2014-04-30 16:01:07]

Adding ch.dumez in reviewers list.

[abarth-chromium, 2014-04-30 16:59:43]

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?

[AviD, 2014-05-02 15:03:22]

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.

[abarth-chromium, 2014-05-02 17:12:45]

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

[AviD, 2014-05-05 04:56:28]

Thanks abarth.
Not sure how that was missed.
PTAL

[abarth-chromium, 2014-05-05 05:14:52]

relgtm

[AviD, 2014-05-05 05:30:22]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2014-05-05 05:30:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/40001

[commit-bot: I haz the power, 2014-05-05 06:05:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-05 06:05:00]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink

[AviD, 2014-05-05 09:58:55]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2014-05-05 09:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/60001

[commit-bot: I haz the power, 2014-05-05 10:59:57]

Change committed as 173295

[loislo, 2014-05-05 12:14:27]

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....

[AviD, 2014-06-02 11:30:42]

@abarth
I have removed the build dependency on
https://codereview.chromium.org/260623004/

Could you PTAL?

[Inactive, 2014-06-02 13:28:34]

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.

[eseidel, 2014-06-02 16:37:50]

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?

[AviD, 2014-06-03 05:53:59]

@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.

[Inactive, 2014-06-09 14:00:51]

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 ':'

[Inactive, 2014-06-09 14:47:10]

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.

[AviD, 2014-06-13 11:52:48]

@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.

[Inactive, 2014-06-13 13:37:24]

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/.

[abarth-chromium, 2014-06-13 17:37:43]

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.

[AviD, 2014-06-20 16:36:44]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2014-06-20 16:37:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/150001

[commit-bot: I haz the power, 2014-06-20 16:37:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:37:22]

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)

[AviD, 2014-06-20 16:50:55]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2014-06-20 16:51:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/170001

[AviD, 2014-06-20 17:56:44]

The CQ bit was checked by avi.nitk@samsung.com

[commit-bot: I haz the power, 2014-06-20 17:58:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi.nitk@samsung.com/259263003/190001

[commit-bot: I haz the power, 2014-06-20 21:24:41]

Change committed as 176646