Add FindInPageBridge

Add FindInPageBridge

First CL to upstream FindInPage functionality.

BUG=332413
Committed: https://crrev.com/54a503abc0b836abbbb3f845954ad007cef3b10d
Cr-Commit-Position: refs/heads/master@{#298801}

[aurimas (slooooooooow), 2014-10-06 22:16:34]

aurimas@chromium.org changed reviewers:
+ tedchoc@chromium.org

[aurimas (slooooooooow), 2014-10-06 22:16:35]

PTAL

[Ted C, 2014-10-07 20:49:50]

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java
(right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:14:
private final long mFindInPageBridge;
I would try to include either the word Native or Ptr into this to make it
obvious it is not a instance of this class itself.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:25:
if (mFindInPageBridge != 0) nativeDestroy(mFindInPageBridge);
You might want to make mFindInPageBridge non-final and set it to 0 here to catch
the asserts you have below.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:32:
return mWebContents != null;
I don't think we should ever support a null web contents in this class.  I would
rather see an assert in the constructor to prevent such behavior.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
File chrome/browser/android/find_in_page/find_in_page_bridge.cc (right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:32: return
reinterpret_cast<intptr_t>(bridge);
And in moving the above static, I would also put this at the bottom

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:41: ->StartFinding(
I "think" the -> goes on the previous line in c++.

Based on my searches in cs.chromium.org:

"""\-\>$ lang:c++""" = 1200
"""^[ ]+\-\> lang:c++""" = 754

That seems to be more common as well.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
File chrome/browser/android/find_in_page/find_in_page_bridge.h (right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.h:18: static bool
RegisterFindInPageBridge(JNIEnv* env);
I would probably put the static at the bottom.

[aurimas (slooooooooow), 2014-10-07 23:28:25]

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java
(right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:14:
private final long mFindInPageBridge;
On 2014/10/07 20:49:49, Ted C wrote:
> I would try to include either the word Native or Ptr into this to make it
> obvious it is not a instance of this class itself.

Done.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:25:
if (mFindInPageBridge != 0) nativeDestroy(mFindInPageBridge);
On 2014/10/07 20:49:49, Ted C wrote:
> You might want to make mFindInPageBridge non-final and set it to 0 here to
catch
> the asserts you have below.

Done.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/findinpage/FindInPageBridge.java:32:
return mWebContents != null;
On 2014/10/07 20:49:49, Ted C wrote:
> I don't think we should ever support a null web contents in this class.  I
would
> rather see an assert in the constructor to prevent such behavior.

Already fixed in patchset 2

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
File chrome/browser/android/find_in_page/find_in_page_bridge.cc (right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:32: return
reinterpret_cast<intptr_t>(bridge);
On 2014/10/07 20:49:49, Ted C wrote:
> And in moving the above static, I would also put this at the bottom

Done.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:41: ->StartFinding(
On 2014/10/07 20:49:49, Ted C wrote:
> I "think" the -> goes on the previous line in c++.
> 
> Based on my searches in http://cs.chromium.org:
> 
> """\-\>$ lang:c++""" = 1200
> """^[ ]+\-\> lang:c++""" = 754
> 
> That seems to be more common as well.

Done.

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
File chrome/browser/android/find_in_page/find_in_page_bridge.h (right):

https://chromiumcodereview.appspot.com/633743002/diff/1/chrome/browser/androi...
chrome/browser/android/find_in_page/find_in_page_bridge.h:18: static bool
RegisterFindInPageBridge(JNIEnv* env);
On 2014/10/07 20:49:49, Ted C wrote:
> I would probably put the static at the bottom.

Done.

[aurimas (slooooooooow), 2014-10-08 17:49:21]

Does this look better now?

[Ted C, 2014-10-08 18:02:27]

lgtm

https://chromiumcodereview.appspot.com/633743002/diff/30001/chrome/browser/an...
File chrome/browser/android/find_in_page/find_in_page_bridge.cc (right):

https://chromiumcodereview.appspot.com/633743002/diff/30001/chrome/browser/an...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:25: static jlong
Init(JNIEnv* env, jobject obj, jobject j_web_contents) {
move this to the bottom as well

[aurimas (slooooooooow), 2014-10-08 18:28:25]

https://codereview.chromium.org/633743002/diff/30001/chrome/browser/android/f...
File chrome/browser/android/find_in_page/find_in_page_bridge.cc (right):

https://codereview.chromium.org/633743002/diff/30001/chrome/browser/android/f...
chrome/browser/android/find_in_page/find_in_page_bridge.cc:25: static jlong
Init(JNIEnv* env, jobject obj, jobject j_web_contents) {
On 2014/10/08 18:02:27, Ted C wrote:
> move this to the bottom as well

Done.

[aurimas (slooooooooow), 2014-10-09 00:37:24]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-10-09 00:43:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/633743002/50001

[commit-bot: I haz the power, 2014-10-09 03:47:25]

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

[commit-bot: I haz the power, 2014-10-09 03:47:25]

Exceeded time limit waiting for builds to trigger.

[aurimas (slooooooooow), 2014-10-09 04:53:19]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-10-09 04:53:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/633743002/50001

[commit-bot: I haz the power, 2014-10-09 05:02:51]

Committed patchset #4 (id:50001) as 6d048df3ff0d07cf27c1c44db893e03340f8dd76

[commit-bot: I haz the power, 2014-10-09 05:03:37]

Patchset 4 (id:??) landed as
https://crrev.com/54a503abc0b836abbbb3f845954ad007cef3b10d
Cr-Commit-Position: refs/heads/master@{#298801}