Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268863004:
Add a return value to TapWebViewElementWithId (Closed)
DescriptionAdd a return value to TapWebViewElementWithId
This CL adds a a return value representing whether the element was found to the
TapWebViewElementWithId method.
BUG=none
Committed: https://crrev.com/95efccaac7e29e98bd6699f6d1288c0df694ee58
Cr-Commit-Position: refs/heads/master@{#414720}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Revision #Patch Set 3 : Rebase #Patch Set 4 : Remove unecessary headers #
Total comments: 18
Patch Set 5 : Revision #Patch Set 6 : cleanup #
Dependent Patchsets: Messages
Total messages: 34 (17 generated)
gambard@chromium.org changed reviewers: + baxley@chromium.org, eugenebut@chromium.org
PTAL.
The CQ bit was checked by gambard@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... File ios/web/public/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:10: #include "base/strings/string_util.h" Toy don't need this, because there is no need for parsing the string https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:27: " if(typeof(element) != 'undefined' && element != null) {" Space after |if| https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:27: " if(typeof(element) != 'undefined' && element != null) {" Would this work?: if (element) {" https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:29: " return 'true';" return true; And then use GetAsBoolean https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:31: " return 'false';" return false; https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:41: value->GetAsString(&str); base::BindBlock(^(const base::Value* value) { if (value) value->GetAsBoolean(&element_found); } }) https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:53: return did_complete && element_found; return element_found; You won't be here if |did_complete| is false.
Thanks! https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... File ios/web/public/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:10: #include "base/strings/string_util.h" On 2016/08/23 15:22:43, Eugene But wrote: > Toy don't need this, because there is no need for parsing the string Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:27: " if(typeof(element) != 'undefined' && element != null) {" On 2016/08/23 15:22:43, Eugene But wrote: > Would this work?: > if (element) {" Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:27: " if(typeof(element) != 'undefined' && element != null) {" On 2016/08/23 15:22:43, Eugene But wrote: > Space after |if| Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:29: " return 'true';" On 2016/08/23 15:22:43, Eugene But wrote: > return true; > > And then use GetAsBoolean Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:31: " return 'false';" On 2016/08/23 15:22:43, Eugene But wrote: > return false; Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:41: value->GetAsString(&str); On 2016/08/23 15:22:43, Eugene But wrote: > base::BindBlock(^(const base::Value* value) { > if (value) > value->GetAsBoolean(&element_found); > } > }) Done. https://codereview.chromium.org/2268863004/diff/1/ios/web/public/test/web_vie... ios/web/public/test/web_view_interaction_test_util.mm:53: return did_complete && element_found; On 2016/08/23 15:22:43, Eugene But wrote: > return element_found; > > You won't be here if |did_complete| is false. Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2268863004/#ps20001 (title: "Revision")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gambard@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...
gambard@chromium.org changed reviewers: + jif@chromium.org
+jif to see if this is compatible with your changes.
jif@google.com changed reviewers: + jif@google.com
lgtm with nit https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:19: // Returns whether the Javascript action specified by |action| run on s/run/ran/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I guess most comments addressed to jif@ :) https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (left): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:12: enum ElementAction { CLICK, FOCUS }; According to Chromium Style Guide these should be ELEMENT_ACTION_CLICK and ELEMENT_ACTION_FOCUS. Could you please fix this. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, Do you see a potential of adding new actions? If you can't think about more than 3 actions then I believe that it is better to have a separate API for each action. Having both TapWebViewElementWithId and RunActionOnWebViewElementWithId (which also allows clicks) seems inconsistent. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:46: element_id.c_str(), jsAction]; Could you please s/jsAction/js_action https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:52: if (!error && result) No need for this check if there is an error |result| will be nil. If |result| is nil then [result boolValue] will return NO;
https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (left): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:12: enum ElementAction { CLICK, FOCUS }; On 2016/08/26 13:19:56, Eugene But wrote: > According to Chromium Style Guide these should be ELEMENT_ACTION_CLICK and > ELEMENT_ACTION_FOCUS. Could you please fix this. Acknowledged. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 13:19:56, Eugene But wrote: > Do you see a potential of adding new actions? If you can't think about more than > 3 actions then I believe that it is better to have a separate API for each > action. Having both TapWebViewElementWithId and RunActionOnWebViewElementWithId > (which also allows clicks) seems inconsistent. And how about removing TapWebViewElementWithId? https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:46: element_id.c_str(), jsAction]; On 2016/08/26 13:19:56, Eugene But wrote: > Could you please s/jsAction/js_action Acknowledged. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:52: if (!error && result) On 2016/08/26 13:19:56, Eugene But wrote: > No need for this check if there is an error |result| will be nil. If |result| is > nil then [result boolValue] will return NO; Acknowledged.
responding to one question. CC cmasso@, who may add the method to tap a link. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 13:30:34, jif-google wrote: > On 2016/08/26 13:19:56, Eugene But wrote: > > Do you see a potential of adding new actions? If you can't think about more > than > > 3 actions then I believe that it is better to have a separate API for each > > action. Having both TapWebViewElementWithId and > RunActionOnWebViewElementWithId > > (which also allows clicks) seems inconsistent. > > And how about removing TapWebViewElementWithId? We're in progress of adding an API that will tap a webview element with link text. I think it's more clear to not have RunActionOnWebViewElementWithId exposed to the user. Even if we have many APIs that use it, as long as the number of APIs is bound, I think it's more clear to have explicit methods for each action, rather than have an open ended one. If we ever have a case (let me know if this is the case) where we can't predict what other action types can be passed in, then I think it's okay to expose it. I think it's good to have RunActionOnWebViewElementWithId to share implementation across the APIs internally.
Ok I have removed the general function as I was not used downstream. PTAL https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (left): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:12: enum ElementAction { CLICK, FOCUS }; On 2016/08/26 13:30:34, jif-google wrote: > On 2016/08/26 13:19:56, Eugene But wrote: > > According to Chromium Style Guide these should be ELEMENT_ACTION_CLICK and > > ELEMENT_ACTION_FOCUS. Could you please fix this. > > Acknowledged. Done. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:19: // Returns whether the Javascript action specified by |action| run on On 2016/08/26 12:20:48, jif-google wrote: > s/run/ran/ Done. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 13:40:05, baxley wrote: > On 2016/08/26 13:30:34, jif-google wrote: > > On 2016/08/26 13:19:56, Eugene But wrote: > > > Do you see a potential of adding new actions? If you can't think about more > > than > > > 3 actions then I believe that it is better to have a separate API for each > > > action. Having both TapWebViewElementWithId and > > RunActionOnWebViewElementWithId > > > (which also allows clicks) seems inconsistent. > > > > And how about removing TapWebViewElementWithId? > > We're in progress of adding an API that will tap a webview element with link > text. I think it's more clear to not have RunActionOnWebViewElementWithId > exposed to the user. > > Even if we have many APIs that use it, as long as the number of APIs is bound, I > think it's more clear to have explicit methods for each action, rather than have > an open ended one. If we ever have a case (let me know if this is the case) > where we can't predict what other action types can be passed in, then I think > it's okay to expose it. > > I think it's good to have RunActionOnWebViewElementWithId to share > implementation across the APIs internally. Done. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:46: element_id.c_str(), jsAction]; On 2016/08/26 13:30:35, jif-google wrote: > On 2016/08/26 13:19:56, Eugene But wrote: > > Could you please s/jsAction/js_action > > Acknowledged. Done. https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.mm:52: if (!error && result) On 2016/08/26 13:30:35, jif-google wrote: > On 2016/08/26 13:19:56, Eugene But wrote: > > No need for this check if there is an error |result| will be nil. If |result| > is > > nil then [result boolValue] will return NO; > > Acknowledged. Done.
LGTM, Thanks!
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, jif@google.com Link to the patchset: https://codereview.chromium.org/2268863004/#ps100001 (title: "cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 13:30:34, jif-google wrote: > On 2016/08/26 13:19:56, Eugene But wrote: > > Do you see a potential of adding new actions? If you can't think about more > than > > 3 actions then I believe that it is better to have a separate API for each > > action. Having both TapWebViewElementWithId and > RunActionOnWebViewElementWithId > > (which also allows clicks) seems inconsistent. > > And how about removing TapWebViewElementWithId? TapWebViewElementWithId seems more clear that RunActionOnWebViewElementWithId https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 13:40:05, baxley wrote: > On 2016/08/26 13:30:34, jif-google wrote: > > On 2016/08/26 13:19:56, Eugene But wrote: > > > Do you see a potential of adding new actions? If you can't think about more > > than > > > 3 actions then I believe that it is better to have a separate API for each > > > action. Having both TapWebViewElementWithId and > > RunActionOnWebViewElementWithId > > > (which also allows clicks) seems inconsistent. > > > > And how about removing TapWebViewElementWithId? > > We're in progress of adding an API that will tap a webview element with link > text. I think it's more clear to not have RunActionOnWebViewElementWithId > exposed to the user. > > Even if we have many APIs that use it, as long as the number of APIs is bound, I > think it's more clear to have explicit methods for each action, rather than have > an open ended one. If we ever have a case (let me know if this is the case) > where we can't predict what other action types can be passed in, then I think > it's okay to expose it. > > I think it's good to have RunActionOnWebViewElementWithId to share > implementation across the APIs internally. +1 for using RunActionOnWebViewElementWithId. We we really need API to tap on link with text? What if many links have the same text? Is there a reason why having TapWebViewElementWithId is not sufficient?
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a return value to TapWebViewElementWithId This CL adds a a return value representing whether the element was found to the TapWebViewElementWithId method. BUG=none ========== to ========== Add a return value to TapWebViewElementWithId This CL adds a a return value representing whether the element was found to the TapWebViewElementWithId method. BUG=none Committed: https://crrev.com/95efccaac7e29e98bd6699f6d1288c0df694ee58 Cr-Commit-Position: refs/heads/master@{#414720} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/95efccaac7e29e98bd6699f6d1288c0df694ee58 Cr-Commit-Position: refs/heads/master@{#414720}
Message was sent while issue was closed.
https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... File ios/web/public/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2268863004/diff/60001/ios/web/public/test/web... ios/web/public/test/web_view_interaction_test_util.h:21: bool RunActionOnWebViewElementWithId(web::WebState* web_state, On 2016/08/26 15:13:28, Eugene But wrote: > On 2016/08/26 13:40:05, baxley wrote: > > On 2016/08/26 13:30:34, jif-google wrote: > > > On 2016/08/26 13:19:56, Eugene But wrote: > > > > Do you see a potential of adding new actions? If you can't think about > more > > > than > > > > 3 actions then I believe that it is better to have a separate API for each > > > > action. Having both TapWebViewElementWithId and > > > RunActionOnWebViewElementWithId > > > > (which also allows clicks) seems inconsistent. > > > > > > And how about removing TapWebViewElementWithId? > > > > We're in progress of adding an API that will tap a webview element with link > > text. I think it's more clear to not have RunActionOnWebViewElementWithId > > exposed to the user. > > > > Even if we have many APIs that use it, as long as the number of APIs is bound, > I > > think it's more clear to have explicit methods for each action, rather than > have > > an open ended one. If we ever have a case (let me know if this is the case) > > where we can't predict what other action types can be passed in, then I think > > it's okay to expose it. > > > > I think it's good to have RunActionOnWebViewElementWithId to share > > implementation across the APIs internally. > +1 for using RunActionOnWebViewElementWithId. > > We we really need API to tap on link with text? What if many links have the same > text? Is there a reason why having TapWebViewElementWithId is not sufficient? As discussed offline... chrome-urls don't have IDs, so the only way to tap them now is via href value. However, we concluded we should pursue adding IDs for the chrome-urls. If there is pushback, or it will take a while, we can add a method to tap link text as a short term workaround. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
