Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159253002:
[system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. (Closed)
Description[system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9.
Washingtonpost popup window on Nexus9 does not have "Close" button.
This patch checks for the presence of the button before clicking it.
BUG=629163
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq
Committed: https://crrev.com/50adac87a62954be18180cbf74590a54a9d70dd4
Cr-Commit-Position: refs/heads/master@{#406372}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix comment, enable benchmarks, check for button #Patch Set 3 : rename variable #
Total comments: 6
Patch Set 4 : Fix style and make selector constant #Patch Set 5 : make constant private #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. Instead, it has "Send link to phone" button. This patch adds selector for the latter button to the click action. BUG=629163 ========== to ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. Instead, it has "Send link to phone" button. This patch adds selector for the latter button to the click action. BUG=629163 ==========
ulan@chromium.org changed reviewers: + petrcermak@chromium.org
ptal
https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:185: # Close the popup window. On Nexus 9 (and probably other tables) the popup How about we do something a little more hacky: action_runner.ExecuteJavaScript('''(function() { var elem = document.querySelector('div#welcome'); if (elem) elem.parentNode.removeChild(elem); })()''');
On 2016/07/19 16:51:58, petrcermak wrote: > https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/browsing_stories.py:185: # Close the popup > window. On Nexus 9 (and probably other tables) the popup > How about we do something a little more hacky: > > action_runner.ExecuteJavaScript('''(function() { > var elem = document.querySelector('div#welcome'); > if (elem) > elem.parentNode.removeChild(elem); > })()'''); Yep, I considered that, but I was worried that it might break invariants in the website and trigger uncommon code path. Closing the popup is not critical: it is transparent and I can see that website navigates between pages. So it is mostly aesthetical issue.
On 2016/07/19 17:01:52, ulan wrote: > On 2016/07/19 16:51:58, petrcermak wrote: > > > https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... > > tools/perf/page_sets/system_health/browsing_stories.py:185: # Close the popup > > window. On Nexus 9 (and probably other tables) the popup > > How about we do something a little more hacky: > > > > action_runner.ExecuteJavaScript('''(function() { > > var elem = document.querySelector('div#welcome'); > > if (elem) > > elem.parentNode.removeChild(elem); > > })()'''); > > Yep, I considered that, but I was worried that it might break invariants in the > website and trigger uncommon code path. > > Closing the popup is not critical: it is transparent and I can see that website > navigates between pages. So it is mostly aesthetical issue. Makes sense. LGTM % a few nits. Thanks, Petr https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:186: # window does not have "Close" button, instead it has only "Send link to supernit: s/have "Close"/have a "Close/ and s/only "Send/only a "Send/ https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:187: # phone" button, which does nothing. So on tablets we run with popup nit: s/popup/the popup/ https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:188: # window open. Please add a sentence (along the lines of your reply): "The popup is transparent, so this is mostly an aesthetical issue." https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/loading_stories.py:177: # Close the popup window. On Nexus 9 (and probably other tables) the popup ditto (everything that applies to the previous comment)
https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:189: action_runner.ClickElement(selector='.close,.mailBlock') On a second thought, I think it would be better not to click any button if there's no "close" button.
Description was changed from ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. Instead, it has "Send link to phone" button. This patch adds selector for the latter button to the click action. BUG=629163 ========== to ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. Instead, it has "Send link to phone" button. This patch adds selector for the latter button to the click action. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ==========
PTAL. I also re-enabled the benchmarks for N9. https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:185: # Close the popup window. On Nexus 9 (and probably other tables) the popup On 2016/07/19 16:51:58, petrcermak wrote: > How about we do something a little more hacky: > > action_runner.ExecuteJavaScript('''(function() { > var elem = document.querySelector('div#welcome'); > if (elem) > elem.parentNode.removeChild(elem); > })()'''); Acknowledged. https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:186: # window does not have "Close" button, instead it has only "Send link to On 2016/07/19 17:07:08, petrcermak wrote: > supernit: s/have "Close"/have a "Close/ and s/only "Send/only a "Send/ Done. https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:187: # phone" button, which does nothing. So on tablets we run with popup On 2016/07/19 17:07:08, petrcermak wrote: > nit: s/popup/the popup/ Done. https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:188: # window open. On 2016/07/19 17:07:08, petrcermak wrote: > Please add a sentence (along the lines of your reply): "The popup is > transparent, so this is mostly an aesthetical issue." Done. https://codereview.chromium.org/2159253002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/browsing_stories.py:189: action_runner.ClickElement(selector='.close,.mailBlock') On 2016/07/19 17:08:48, petrcermak wrote: > On a second thought, I think it would be better not to click any button if > there's no "close" button. Done. Added a check.
LGTM with 2 more comments. Also please update the patch description appropriately (it's not the case that the send email button is clicked anymore). Thanks, Petr https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:189: buttonSelector = '.close' button_selector. Furthermore, this could be a class constant (_CLOSE_BUTTON_SELECTOR) https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:190: hasButton = action_runner.EvaluateJavaScript( has_button https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/loading_stories.py:181: buttonSelector = '.close' Same comments as above.
ulan@chromium.org changed reviewers: + nednguyen@google.com
Thanks! +Ned for owners rubber-stamp. https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:189: buttonSelector = '.close' On 2016/07/19 17:41:24, petrcermak wrote: > button_selector. Furthermore, this could be a class constant > (_CLOSE_BUTTON_SELECTOR) Done. https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/browsing_stories.py:190: hasButton = action_runner.EvaluateJavaScript( On 2016/07/19 17:41:24, petrcermak wrote: > has_button Done. https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/loading_stories.py (right): https://codereview.chromium.org/2159253002/diff/40001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/loading_stories.py:181: buttonSelector = '.close' On 2016/07/19 17:41:24, petrcermak wrote: > Same comments as above. Done.
lgtm
Description was changed from ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. Instead, it has "Send link to phone" button. This patch adds selector for the latter button to the click action. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. This patch checks for the presence of the button before clicking it. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2159253002/#ps80001 (title: "make constant private")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. This patch checks for the presence of the button before clicking it. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. This patch checks for the presence of the button before clicking it. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. This patch checks for the presence of the button before clicking it. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [system health] Fix load:news:washingtonpost and browse:news:washingtonpost on Nexus9. Washingtonpost popup window on Nexus9 does not have "Close" button. This patch checks for the presence of the button before clicking it. BUG=629163 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/50adac87a62954be18180cbf74590a54a9d70dd4 Cr-Commit-Position: refs/heads/master@{#406372} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/50adac87a62954be18180cbf74590a54a9d70dd4 Cr-Commit-Position: refs/heads/master@{#406372} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
