Add back buttons and load new 'pages' by calling javascript.

Add back buttons and load new 'pages' by calling javascript.

Previously when we want to see the details inside a specific suite, we
will go to a url '.../?suite=xxx'. This may not work in google storage.

In this cl, I am changing the logic by calling javascript directly to
change the content of the page, if the user wants to see a specific
suite.

Because we are not changing urls anymore, we need to have a "Back"
button to go back to the main page.

BUG=605572
Review-Url: https://codereview.chromium.org/2799153008
Cr-Commit-Position: refs/heads/master@{#463781}
Committed: https://chromium.googlesource.com/chromium/src/+/9b4e899d21ec602a49cd050b800085cd22130c69

[BigBossZhiling, 2017-04-07 22:08:26]

Description was changed from

==========
Add back buttons and load new 'pages' by calling javascript.

Previously when we want to see the details inside a specific suite, we
will go to a url '.../?suite=xxx'. This may not work in google storage.

In this cl, I am changing the logic by calling javascript directly to
change the content of the page, if the user wants to see a specific
suite.

Because we are not changing urls anymore, we need to have a "Back"
button to go back to the main page.

BUG=605572
==========

to

==========
Add back buttons and load new 'pages' by calling javascript.

Previously when we want to see the details inside a specific suite, we
will go to a url '.../?suite=xxx'. This may not work in google storage.

In this cl, I am changing the logic by calling javascript directly to
change the content of the page, if the user wants to see a specific
suite.

Because we are not changing urls anymore, we need to have a "Back"
button to go back to the main page.

BUG=605572
==========

[BigBossZhiling, 2017-04-07 22:08:26]

hzl@google.com changed reviewers:
+ jbudorick@chromium.org, mikecase@chromium.org

[mikecase (-- gone --), 2017-04-07 23:57:20]

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
File build/android/pylib/results/presentation/template/main.html (right):

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/template/main.html:47: <a
class="back_button" onclick="showSuiteTableOnly();"><b><--
Back</b></br></br></a>
ooooooh, I forgot we will lose the ability to use browser back button. I think
instead of this solution you should just use...

history.pushState()  and also hook into the window.onpopstate event.

https://developer.mozilla.org/en-US/docs/Web/Events/popstate

Then you can get the back button working, but it wont actually load a new page,
so Google storage website wont give you the authentication error thing.

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
File build/android/pylib/results/presentation/test_results_presentation.py
(right):

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:81: def
action_cell(action, data, html_class):
I do not think we should pass javascript functions in as strings.

[mikecase (-- gone --), 2017-04-10 17:03:07]

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
File build/android/pylib/results/presentation/test_results_presentation.py
(right):

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:185:
'showTestsOfOneSuiteOnly(\'TOTAL\')',
nit: use double quotes here instead of \'

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:209:
'showTestsOfOneSuiteOnly(\'%s\')' % suite_name,
same here

[BigBossZhiling, 2017-04-10 19:43:52]

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
File build/android/pylib/results/presentation/template/main.html (right):

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/template/main.html:47: <a
class="back_button" onclick="showSuiteTableOnly();"><b><--
Back</b></br></br></a>
On 2017/04/07 23:57:20, mikecase wrote:
> ooooooh, I forgot we will lose the ability to use browser back button. I think
> instead of this solution you should just use...
> 
> history.pushState()  and also hook into the window.onpopstate event.
> 
> https://developer.mozilla.org/en-US/docs/Web/Events/popstate
> 
> Then you can get the back button working, but it wont actually load a new
page,
> so Google storage website wont give you the authentication error thing.

Done.

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
File build/android/pylib/results/presentation/test_results_presentation.py
(right):

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:81: def
action_cell(action, data, html_class):
On 2017/04/07 23:57:20, mikecase wrote:
> I do not think we should pass javascript functions in as strings.

Acknowledged.

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:185:
'showTestsOfOneSuiteOnly(\'TOTAL\')',
On 2017/04/10 17:03:06, mikecase wrote:
> nit: use double quotes here instead of \'

Done.

https://codereview.chromium.org/2799153008/diff/20001/build/android/pylib/res...
build/android/pylib/results/presentation/test_results_presentation.py:209:
'showTestsOfOneSuiteOnly(\'%s\')' % suite_name,
On 2017/04/10 17:03:06, mikecase wrote:
> same here

Done.

[jbudorick, 2017-04-11 17:52:47]

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
File build/android/pylib/results/presentation/javascript/main_html.js (right):

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
build/android/pylib/results/presentation/javascript/main_html.js:49:
window.scrollTo(0, 0);
Why are we now scrolling to the top?

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
build/android/pylib/results/presentation/javascript/main_html.js:50: if
(push_state) {
Should this & replace_state be in these two functions? What if we extracted
them, either to the call sites directly or to separate (or wrapping) functions?

[chromium-reviews, 2017-04-11 17:58:25]

The reason now we are scrolling to the top is that, the browser thinks we
are still on the same page because we didn't change the url and reload, as
we previously did when we add ?xxx=yyy and reload. As a result, the browser
will stick to the "same" place. Therefore if we were on the bottom of the
suite table page, when we click to see the test table, we will still be on
the bottom part of the page.

On Tue, Apr 11, 2017 at 10:52 AM, <jbudorick@chromium.org> wrote:

>
> https://codereview.chromium.org/2799153008/diff/160001/
> build/android/pylib/results/presentation/javascript/main_html.js
> File build/android/pylib/results/presentation/javascript/main_html.js
> (right):
>
> https://codereview.chromium.org/2799153008/diff/160001/
> build/android/pylib/results/presentation/javascript/main_html.js#newcode49
> build/android/pylib/results/presentation/javascript/main_html.js:49:
> window.scrollTo(0, 0);
> Why are we now scrolling to the top?
>
> https://codereview.chromium.org/2799153008/diff/160001/
> build/android/pylib/results/presentation/javascript/main_html.js#newcode50
> build/android/pylib/results/presentation/javascript/main_html.js:50: if
> (push_state) {
> Should this & replace_state be in these two functions? What if we
> extracted them, either to the call sites directly or to separate (or
> wrapping) functions?
>
> https://codereview.chromium.org/2799153008/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[BigBossZhiling, 2017-04-11 18:37:51]

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
File build/android/pylib/results/presentation/javascript/main_html.js (right):

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
build/android/pylib/results/presentation/javascript/main_html.js:49:
window.scrollTo(0, 0);
On 2017/04/11 17:52:47, jbudorick wrote:
> Why are we now scrolling to the top?

Acknowledged.

https://codereview.chromium.org/2799153008/diff/160001/build/android/pylib/re...
build/android/pylib/results/presentation/javascript/main_html.js:50: if
(push_state) {
On 2017/04/11 17:52:47, jbudorick wrote:
> Should this & replace_state be in these two functions? What if we extracted
> them, either to the call sites directly or to separate (or wrapping)
functions?

Done.

[jbudorick, 2017-04-11 18:43:02]

https://codereview.chromium.org/2799153008/diff/180001/build/android/pylib/re...
File build/android/pylib/results/presentation/javascript/main_html.js (right):

https://codereview.chromium.org/2799153008/diff/180001/build/android/pylib/re...
build/android/pylib/results/presentation/javascript/main_html.js:52: function
testTablePage(suite_name, push_state) {
My motivation w/ this suggestion was to eliminate the flag entirely. If a client
wants to push the state, they call this; otherwise, they call
showTestsOfOneSuiteOnly. (Same w/ suiteTablePage vs showSuiteTableOnly)

[BigBossZhiling, 2017-04-11 18:52:22]

On 2017/04/11 18:43:02, jbudorick wrote:
>
https://codereview.chromium.org/2799153008/diff/180001/build/android/pylib/re...
> File build/android/pylib/results/presentation/javascript/main_html.js (right):
> 
>
https://codereview.chromium.org/2799153008/diff/180001/build/android/pylib/re...
> build/android/pylib/results/presentation/javascript/main_html.js:52: function
> testTablePage(suite_name, push_state) {
> My motivation w/ this suggestion was to eliminate the flag entirely. If a
client
> wants to push the state, they call this; otherwise, they call
> showTestsOfOneSuiteOnly. (Same w/ suiteTablePage vs showSuiteTableOnly)

Done.

[jbudorick, 2017-04-11 18:55:18]

lgtm

[BigBossZhiling, 2017-04-11 18:57:43]

The CQ bit was checked by hzl@google.com

[commit-bot: I haz the power, 2017-04-11 18:58:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[BigBossZhiling, 2017-04-11 19:03:21]

The CQ bit was unchecked by hzl@google.com

[BigBossZhiling, 2017-04-11 20:16:53]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-04-11 20:16:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2799153008/#ps220001
(title: "make bucket argument mandatory")

[commit-bot: I haz the power, 2017-04-11 20:17:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 21:42:23]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491941813475580,
"parent_rev": "41fb39bbab83396812415b5b303c3ca7b47c2bd4", "commit_rev":
"9b4e899d21ec602a49cd050b800085cd22130c69"}

[commit-bot: I haz the power, 2017-04-11 21:43:34]

Description was changed from

==========
Add back buttons and load new 'pages' by calling javascript.

Previously when we want to see the details inside a specific suite, we
will go to a url '.../?suite=xxx'. This may not work in google storage.

In this cl, I am changing the logic by calling javascript directly to
change the content of the page, if the user wants to see a specific
suite.

Because we are not changing urls anymore, we need to have a "Back"
button to go back to the main page.

BUG=605572
==========

to

==========
Add back buttons and load new 'pages' by calling javascript.

Previously when we want to see the details inside a specific suite, we
will go to a url '.../?suite=xxx'. This may not work in google storage.

In this cl, I am changing the logic by calling javascript directly to
change the content of the page, if the user wants to see a specific
suite.

Because we are not changing urls anymore, we need to have a "Back"
button to go back to the main page.

BUG=605572

Review-Url: https://codereview.chromium.org/2799153008
Cr-Commit-Position: refs/heads/master@{#463781}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b4e899d21ec602a49cd050b8000...
==========

[commit-bot: I haz the power, 2017-04-11 21:43:35]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/9b4e899d21ec602a49cd050b8000...

[BigBossZhiling, 2017-04-11 21:58:56]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2814903002/ by hzl@google.com.

The reason for reverting is: Reverting because it broke stuff..