Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724363004:
Content class factory and generalized text selector (Closed)
|
Created:
3 years, 9 months ago by Tima Vaisburd Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionContent class factory and generalized text selector
This CL adds new ContextSelectionProvider interface.
The object that implements this interface needs to
be instantiated with a content browser factory.
BUG=685420
Review-Url: https://codereview.chromium.org/2724363004
Cr-Commit-Position: refs/heads/master@{#455126}
Committed: https://chromium.googlesource.com/chromium/src/+/cfd3fb474d3d26674c5e1329d5db953ea2be3ee5
Patch Set 1 #
Total comments: 22
Patch Set 2 : Interface cleanup #
Total comments: 8
Patch Set 3 : Addressed comments #
Messages
Total messages: 28 (11 generated)
timav@chromium.org changed reviewers: + boliu@chromium.org, sgurun@google.com
PTAL https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:119: private ContextSelectionProvider mContextSelector; Now I think that having both mContextualSearchClient and mContextSelector is wrong, which means a new interface is only needed for the callback part. This |mContextSelector| is to show the use of the factory only, to be removed.
Description was changed from ========== Content class factory and generalized text selector This CL adds new ContextSelectionProvider interface. The object that implements this interface needs to be instantiated with a content browser factory. BUG=685420 ========== to ========== Content class factory and generalized text selector This CL adds new ContextSelectionProvider interface. The object that implements this interface needs to be instantiated with a content browser factory. BUG=685420 ==========
sgurun@chromium.org changed reviewers: + sgurun@chromium.org - sgurun@google.com
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:16: nit: no empty line https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. "Negative number means moving left." one of these comments is wrong? https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: public interface Callback { nit: ResultCallback? https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:70: public void sendSuggestAndClassifyRequest(); same question as before, how does provider get the text in this interface? https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java:5: package org.chromium.content.browser.factory; why different package? so there's some layering things here. although it's not yet enforced and there's lots of violations, embedder should only import from content_public Embedder only needs "void installFactory()", so that's the only part that should be in content_public. The actual factory and provider implementation can be in content. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java:34: public ContextSelectionProvider createContextSelectionProvider( abstract
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: public interface Callback { On 2017/03/03 18:19:09, boliu wrote: > nit: ResultCallback? Thank you for prompt review, I was waiting for your response before making further modifications because I now think that this ContextSelectionProvider interface does not actually makes sense, because we would like to derive the manager from (renamed) ContextualSearchClient instead. Otherwise I would end up having both mContextualSearchClient and mContextSelectionProvider in SelectonPopupController which I think is undesirable. Instead I am going to combine the Result and onClassified(Result) into a separate callback interface, as before. WDYT? Other answers will follow.
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: public interface Callback { On 2017/03/03 18:30:29, Tima Vaisburd wrote: > On 2017/03/03 18:19:09, boliu wrote: > > nit: ResultCallback? > > Thank you for prompt review, I was waiting for your response before making > further modifications because I now think that this ContextSelectionProvider > interface does not actually makes sense, because we would like to derive the > manager from (renamed) ContextualSearchClient instead. Otherwise I would end up > having both mContextualSearchClient and mContextSelectionProvider in > SelectonPopupController which I think is undesirable. > > Instead I am going to combine the Result and onClassified(Result) into a > separate callback interface, as before. WDYT? > > Other answers will follow. I don't know how similar those two will be, but what does that mean for chrome if chrome wants to support both contextual search and this thing?
On 2017/03/03 18:34:54, boliu wrote: > https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java > (right): > > https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: > public interface Callback { > On 2017/03/03 18:30:29, Tima Vaisburd wrote: > > On 2017/03/03 18:19:09, boliu wrote: > > > nit: ResultCallback? > > > > Thank you for prompt review, I was waiting for your response before making > > further modifications because I now think that this ContextSelectionProvider > > interface does not actually makes sense, because we would like to derive the > > manager from (renamed) ContextualSearchClient instead. Otherwise I would end > up > > having both mContextualSearchClient and mContextSelectionProvider in > > SelectonPopupController which I think is undesirable. > > > > Instead I am going to combine the Result and onClassified(Result) into a > > separate callback interface, as before. WDYT? > > > > Other answers will follow. > > I don't know how similar those two will be, but what does that mean for chrome > if chrome wants to support both contextual search and this thing? Chrome people wanted to see what we did and build upon this. They explicitly asked me to concentrate on WebView first. I believe their current idea is subclassing the result of our work, in any case I got a negative feedback on two clients.
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. On 2017/03/03 18:19:09, boliu wrote: > "Negative number means moving left." one of these comments is wrong? M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary should remain intact, -1 means moving one char left and +1 - one char right. Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means expansion in both directions. Where is a contradiction? https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:70: public void sendSuggestAndClassifyRequest(); On 2017/03/03 18:19:09, boliu wrote: > same question as before, how does provider get the text in this interface? Provider will issue the IPC request to blink::WebSurroundingText::initializeFromCurrentSelection() https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSurroun... In other words, I plan the provider to query the Blink/Renderer based on the existing selection.
On 2017/03/03 18:38:45, Tima Vaisburd wrote: > On 2017/03/03 18:34:54, boliu wrote: > > > https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java > > (right): > > > > > https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... > > > content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: > > public interface Callback { > > On 2017/03/03 18:30:29, Tima Vaisburd wrote: > > > On 2017/03/03 18:19:09, boliu wrote: > > > > nit: ResultCallback? > > > > > > Thank you for prompt review, I was waiting for your response before making > > > further modifications because I now think that this ContextSelectionProvider > > > interface does not actually makes sense, because we would like to derive the > > > manager from (renamed) ContextualSearchClient instead. Otherwise I would end > > up > > > having both mContextualSearchClient and mContextSelectionProvider in > > > SelectonPopupController which I think is undesirable. > > > > > > Instead I am going to combine the Result and onClassified(Result) into a > > > separate callback interface, as before. WDYT? > > > > > > Other answers will follow. > > > > I don't know how similar those two will be, but what does that mean for chrome > > if chrome wants to support both contextual search and this thing? > > Chrome people wanted to see what we did and build upon this. They explicitly > asked me to > concentrate on WebView first. I believe their current idea is subclassing the > result of our work, > in any case I got a negative feedback on two clients. Whatever. It's their problem to integrate this I guess https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. On 2017/03/03 18:59:44, Tima Vaisburd wrote: > On 2017/03/03 18:19:09, boliu wrote: > > "Negative number means moving left." one of these comments is wrong? > > M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary > should remain intact, -1 means moving one char left and +1 - one char right. > Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means expansion in > both directions. Where is a contradiction? I mean vs the comment on rightAdjust? https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:70: public void sendSuggestAndClassifyRequest(); On 2017/03/03 18:59:44, Tima Vaisburd wrote: > On 2017/03/03 18:19:09, boliu wrote: > > same question as before, how does provider get the text in this interface? > > Provider will issue the IPC request to > blink::WebSurroundingText::initializeFromCurrentSelection() > https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSurroun... > > In other words, I plan the provider to query the Blink/Renderer based on the > existing selection. Don't do that. Just pass whatever provider needs here. Don't add unnecessary logic downstream.
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. On 2017/03/03 19:44:00, boliu wrote: > On 2017/03/03 18:59:44, Tima Vaisburd wrote: > > On 2017/03/03 18:19:09, boliu wrote: > > > "Negative number means moving left." one of these comments is wrong? > > > > M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary > > should remain intact, -1 means moving one char left and +1 - one char right. > > Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means expansion > in > > both directions. Where is a contradiction? > > I mean vs the comment on rightAdjust? But... rightAdjust can also be negative. For instance, (leftAdjust = -1, rightAdjust = -1) would shift the whole selection one character to the left. It says "Negative number means moving left". Maybe provide examples? Or maybe this: rightAdjust does not mean "direction to the right", it means "the right end of the interval". https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:70: public void sendSuggestAndClassifyRequest(); On 2017/03/03 19:44:00, boliu wrote: > On 2017/03/03 18:59:44, Tima Vaisburd wrote: > > On 2017/03/03 18:19:09, boliu wrote: > > > same question as before, how does provider get the text in this interface? > > > > Provider will issue the IPC request to > > blink::WebSurroundingText::initializeFromCurrentSelection() > > > https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSurroun... > > > > In other words, I plan the provider to query the Blink/Renderer based on the > > existing selection. > > Don't do that. Just pass whatever provider needs here. Don't add unnecessary > logic downstream. Yes, that also answers my question about reusing ContextualSearchClient interface. We can't do that here, I believe we need a class between SelectionPopupController and the one that implements this interface.
https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. On 2017/03/03 20:57:48, Tima Vaisburd wrote: > On 2017/03/03 19:44:00, boliu wrote: > > On 2017/03/03 18:59:44, Tima Vaisburd wrote: > > > On 2017/03/03 18:19:09, boliu wrote: > > > > "Negative number means moving left." one of these comments is wrong? > > > > > > M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary > > > should remain intact, -1 means moving one char left and +1 - one char right. > > > Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means expansion > > in > > > both directions. Where is a contradiction? > > > > I mean vs the comment on rightAdjust? > > But... rightAdjust can also be negative. For instance, (leftAdjust = -1, > rightAdjust = -1) would shift the whole selection one character to the left. It > says "Negative number means moving left". Maybe provide examples? > > Or maybe this: rightAdjust does not mean "direction to the right", it means "the > right end of the interval". hmm, yeah, start/end might be better var names then?
I think I addressed everything except installFactory() in content_public. Please take another look. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:16: On 2017/03/03 18:19:09, boliu wrote: > nit: no empty line Done. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:25: * selection should be moved. Negative number means moving left. On 2017/03/03 22:04:12, boliu wrote: > On 2017/03/03 20:57:48, Tima Vaisburd wrote: > > On 2017/03/03 19:44:00, boliu wrote: > > > On 2017/03/03 18:59:44, Tima Vaisburd wrote: > > > > On 2017/03/03 18:19:09, boliu wrote: > > > > > "Negative number means moving left." one of these comments is wrong? > > > > > > > > M-mm, not sure how to explain it better. If leftAdjust == 0, left boundary > > > > should remain intact, -1 means moving one char left and +1 - one char > right. > > > > Same with right boundary. (leftAdjust = -1, rightAdjust = 1) means > expansion > > > in > > > > both directions. Where is a contradiction? > > > > > > I mean vs the comment on rightAdjust? > > > > But... rightAdjust can also be negative. For instance, (leftAdjust = -1, > > rightAdjust = -1) would shift the whole selection one character to the left. > It > > says "Negative number means moving left". Maybe provide examples? > > > > Or maybe this: rightAdjust does not mean "direction to the right", it means > "the > > right end of the interval". > > hmm, yeah, start/end might be better var names then? Done. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:59: public interface Callback { On 2017/03/03 18:19:09, boliu wrote: > nit: ResultCallback? Done. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:70: public void sendSuggestAndClassifyRequest(); On 2017/03/03 20:57:48, Tima Vaisburd wrote: > On 2017/03/03 19:44:00, boliu wrote: > > On 2017/03/03 18:59:44, Tima Vaisburd wrote: > > > On 2017/03/03 18:19:09, boliu wrote: > > > > same question as before, how does provider get the text in this interface? > > > > > > Provider will issue the IPC request to > > > blink::WebSurroundingText::initializeFromCurrentSelection() > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebSurroun... > > > > > > In other words, I plan the provider to query the Blink/Renderer based on the > > > existing selection. > > > > Don't do that. Just pass whatever provider needs here. Don't add unnecessary > > logic downstream. > > Yes, that also answers my question about reusing ContextualSearchClient > interface. We can't do that here, I believe we need a class between > SelectionPopupController and the one that implements this interface. Added the text and selection within it as arguments. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java (right): https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java:5: package org.chromium.content.browser.factory; On 2017/03/03 18:19:09, boliu wrote: > why different package? > > so there's some layering things here. although it's not yet enforced and there's > lots of violations, embedder should only import from content_public > > Embedder only needs "void installFactory()", so that's the only part that should > be in content_public. The actual factory and provider implementation can be in > content. This is not done yet. https://codereview.chromium.org/2724363004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/factory/ContentClassFactory.java:34: public ContextSelectionProvider createContextSelectionProvider( On 2017/03/03 18:19:09, boliu wrote: > abstract Instead I decides to have a regular |if (!signleton) singleton = new Singleton();| so ContentClassFactory.get() is never null.
https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:27: // Should be called on the Browser UI thread. then assert, ThreadUtils has a method to do that iirc https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:35: public ContextSelectionProvider createContextSelectionProvider( add a protected constructor https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:69: * @param start The start index of the selected text inside the textual context. probably want to specify whether these indices are inclusive or exclusive https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:151: // To be removed. should be a TODO comment then. And need to give reasoning
https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:27: // Should be called on the Browser UI thread. On 2017/03/06 16:56:13, boliu wrote: > then assert, ThreadUtils has a method to do that iirc Done. https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentClassFactory.java:35: public ContextSelectionProvider createContextSelectionProvider( On 2017/03/06 16:56:13, boliu wrote: > add a protected constructor Done. https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextSelectionProvider.java:69: * @param start The start index of the selected text inside the textual context. On 2017/03/06 16:56:13, boliu wrote: > probably want to specify whether these indices are inclusive or exclusive Rewrote the explanation. |start| is inclusive and |end| in exclusive. https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2724363004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:151: // To be removed. On 2017/03/06 16:56:13, boliu wrote: > should be a TODO comment then. And need to give reasoning Just removed.
lgtm
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by timav@chromium.org
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488904044923440,
"parent_rev": "56c267d43381c48f5fc1d1acf43550e2cd3e759a", "commit_rev":
"cfd3fb474d3d26674c5e1329d5db953ea2be3ee5"}
Message was sent while issue was closed.
Description was changed from ========== Content class factory and generalized text selector This CL adds new ContextSelectionProvider interface. The object that implements this interface needs to be instantiated with a content browser factory. BUG=685420 ========== to ========== Content class factory and generalized text selector This CL adds new ContextSelectionProvider interface. The object that implements this interface needs to be instantiated with a content browser factory. BUG=685420 Review-Url: https://codereview.chromium.org/2724363004 Cr-Commit-Position: refs/heads/master@{#455126} Committed: https://chromium.googlesource.com/chromium/src/+/cfd3fb474d3d26674c5e1329d5db... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/cfd3fb474d3d26674c5e1329d5db... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
