Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
444143002:
Loading Indicator for Distilled Pages (Closed)
|
Created:
6 years, 4 months ago by sunangel Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, smaslo Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionLoading Indicator for Distilled Pages.
Adds loading indicator for long distilled pages.
BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289601
Patch Set 1 : #Patch Set 2 : Reverted partial article loading indicator default #
Total comments: 20
Patch Set 3 : Added closure #
Total comments: 6
Patch Set 4 : invoke updateLoadingIndicator on page load #Patch Set 5 : Inverted to isLastPage #
Total comments: 6
Patch Set 6 : styling #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:16: else { } else { https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:36: function updateLoadingIndicator(isNotLastPage) { isNotLastPage should be treated as true or false. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:37: if (isNotLastPage == 1 && this.alreadyRunning == 0) { No need to compare to == 1 or == 0. http://www.ecma-international.org/ecma-262/5.1/#sec-9.2 https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: var colorIndex = -1; Move this declaration into the if (loader) block. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:39: var colors = ["red", "yellow", "green", "blue"]; Put this in a private scope. var updateLoadingIndicator = function() { var colors = ["red", "yellow", "green", "blue"]; return function(isNotLastPage) { if (isNotLastPage ...) }; }(); https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:41: if(loader) { if (loader) https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:50: clearInterval(this.colorShuffle); 2 spaces for tabs in JS. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:56: this.alreadyRunning = 0; Instead of alreadyRunning, you can use the existence of colorShuffle to determine if the interval is running. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/core/css/distilledpage.css (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/core/css/distilledpage.css:293: -moz-transition-delay: 0.22s; Do we still need the -moz declarations since this is chrome only? https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/core/viewer.cc:118: title, unsafe_article_html, "visible", original_url, theme); This line didn't change, so I would leave it alone.
https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:16: else { On 2014/08/07 17:58:07, robliao wrote: > } else { Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:36: function updateLoadingIndicator(isNotLastPage) { On 2014/08/07 17:58:07, robliao wrote: > isNotLastPage should be treated as true or false. Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:37: if (isNotLastPage == 1 && this.alreadyRunning == 0) { On 2014/08/07 17:58:07, robliao wrote: > No need to compare to == 1 or == 0. > > http://www.ecma-international.org/ecma-262/5.1/#sec-9.2 Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: var colorIndex = -1; On 2014/08/07 17:58:07, robliao wrote: > Move this declaration into the if (loader) block. Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:39: var colors = ["red", "yellow", "green", "blue"]; Wow did not know you could do scoping in javascript. Thanks! On 2014/08/07 17:58:07, robliao wrote: > Put this in a private scope. > > var updateLoadingIndicator = function() { > var colors = ["red", "yellow", "green", "blue"]; > return function(isNotLastPage) { > if (isNotLastPage ...) > }; > }(); https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:41: if(loader) { On 2014/08/07 17:58:07, robliao wrote: > if (loader) Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:50: clearInterval(this.colorShuffle); On 2014/08/07 17:58:07, robliao wrote: > 2 spaces for tabs in JS. Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:56: this.alreadyRunning = 0; On 2014/08/07 17:58:07, robliao wrote: > Instead of alreadyRunning, you can use the existence of colorShuffle to > determine if the interval is running. Done. https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/core/css/distilledpage.css (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/core/css/distilledpage.css:293: -moz-transition-delay: 0.22s; oh yeah oops! I think in that case we also don't need the webkit. On 2014/08/07 17:58:07, robliao wrote: > Do we still need the -moz declarations since this is chrome only? https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... File components/dom_distiller/core/viewer.cc (right): https://codereview.chromium.org/444143002/diff/110001/components/dom_distille... components/dom_distiller/core/viewer.cc:118: title, unsafe_article_html, "visible", original_url, theme); On 2014/08/07 17:58:07, robliao wrote: > This line didn't change, so I would leave it alone. Done.
https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: return function(isNotLastPage) { If you inverted this to isLastPage, you can pass the argument of showLoadingIndicator directly to here instead of a direct call of true/false. https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:52: }; Don't forget to invoke it (note the () at the end of the comment before)! Right now updateLoading indicator points to the anonymous function with no args. We want the one inside, so we have to call this anonymous function first.
https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: return function(isNotLastPage) { inverted it in function call above instead...is that okay? On 2014/08/07 21:53:04, robliao wrote: > If you inverted this to isLastPage, you can pass the argument of > showLoadingIndicator directly to here instead of a direct call of true/false. https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:52: }; On 2014/08/07 21:53:04, robliao wrote: > Don't forget to invoke it (note the () at the end of the comment before)! Right > now updateLoading indicator points to the anonymous function with no args. We > want the one inside, so we have to call this anonymous function first. Done.
https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: return function(isNotLastPage) { On 2014/08/08 00:45:30, sunangel wrote: > inverted it in function call above instead...is that okay? > On 2014/08/07 21:53:04, robliao wrote: > > If you inverted this to isLastPage, you can pass the argument of > > showLoadingIndicator directly to here instead of a direct call of true/false. > Right. The point here is that this function can also accept isLastPage, making this consistent with the above. Then your calls above become loadingIndicatorShuffle(isLastPage) and then you can invert your logic in this function.
https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:38: return function(isNotLastPage) { Wait hm okay sorry. The change I thought I made didn't show up -__-. I reuploaded it. On 2014/08/08 17:22:30, robliao wrote: > On 2014/08/08 00:45:30, sunangel wrote: > > inverted it in function call above instead...is that okay? > > On 2014/08/07 21:53:04, robliao wrote: > > > If you inverted this to isLastPage, you can pass the argument of > > > showLoadingIndicator directly to here instead of a direct call of > true/false. > > > > Right. The point here is that this function can also accept isLastPage, making > this consistent with the above. > > Then your calls above become loadingIndicatorShuffle(isLastPage) and then you > can invert your logic in this function.
On 2014/08/08 21:25:04, sunangel wrote: > https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... > File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): > > https://codereview.chromium.org/444143002/diff/130001/components/dom_distille... > components/dom_distiller/content/resources/dom_distiller_viewer.js:38: return > function(isNotLastPage) { > Wait hm okay sorry. The change I thought I made didn't show up -__-. I > reuploaded it. > On 2014/08/08 17:22:30, robliao wrote: > > On 2014/08/08 00:45:30, sunangel wrote: > > > inverted it in function call above instead...is that okay? > > > On 2014/08/07 21:53:04, robliao wrote: > > > > If you inverted this to isLastPage, you can pass the argument of > > > > showLoadingIndicator directly to here instead of a direct call of > > true/false. > > > > > > > Right. The point here is that this function can also accept isLastPage, making > > this consistent with the above. > > > > Then your calls above become loadingIndicatorShuffle(isLastPage) and then you > > can invert your logic in this function. Go ahead and loop in nyquist@
https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:12: if (isLastPage) { Could this just be something short like this? document.getElementById('loadingIndicator').className = isLastPage ? 'hidden' : 'visible'; updateLoadingIndicator(isLastPage); Or is the ordering of setting the classname and updating the loading indicator important (since you had different ordering in if and else branches). I could see that you might have to "create"/update the loading indicator before showing it.
https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:12: if (isLastPage) { I think it would be better if it were ordered like this so that the icon is immediately hidden first and so that by the time you show the loading indicator, it's already started. On 2014/08/11 20:47:05, nyquist wrote: > Could this just be something short like this? > > document.getElementById('loadingIndicator').className = > isLastPage ? 'hidden' : 'visible'; > updateLoadingIndicator(isLastPage); > > Or is the ordering of setting the classname and updating the loading indicator > important (since you had different ordering in if and else branches). I could > see that you might have to "create"/update the loading indicator before showing > it.
On 2014/08/12 01:56:08, sunangel wrote: > https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... > File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): > > https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... > components/dom_distiller/content/resources/dom_distiller_viewer.js:12: if > (isLastPage) { > I think it would be better if it were ordered like this so that the icon is > immediately hidden first and so that by the time you show the loading indicator, > it's already started. > On 2014/08/11 20:47:05, nyquist wrote: > > Could this just be something short like this? > > > > document.getElementById('loadingIndicator').className = > > isLastPage ? 'hidden' : 'visible'; > > updateLoadingIndicator(isLastPage); > > > > Or is the ordering of setting the classname and updating the loading indicator > > important (since you had different ordering in if and else branches). I could > > see that you might have to "create"/update the loading indicator before > showing > > it. lgtm.
https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:12: if (isLastPage) { On 2014/08/12 01:56:08, sunangel wrote: > I think it would be better if it were ordered like this so that the icon is > immediately hidden first and so that by the time you show the loading indicator, > it's already started. > On 2014/08/11 20:47:05, nyquist wrote: > > Could this just be something short like this? > > > > document.getElementById('loadingIndicator').className = > > isLastPage ? 'hidden' : 'visible'; > > updateLoadingIndicator(isLastPage); > > > > Or is the ordering of setting the classname and updating the loading indicator > > important (since you had different ordering in if and else branches). I could > > see that you might have to "create"/update the loading indicator before > showing > > it. > AFAIK the interval only triggers after 600 ms, and not instantly, so I think you should simplify this. See my previous comment. https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:43: colorIndex = (colorIndex + 1) % colors.length; I think these two lines are missing indentation.
https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... File components/dom_distiller/content/resources/dom_distiller_viewer.js (right): https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:12: if (isLastPage) { On 2014/08/12 22:04:32, nyquist wrote: > On 2014/08/12 01:56:08, sunangel wrote: > > I think it would be better if it were ordered like this so that the icon is > > immediately hidden first and so that by the time you show the loading > indicator, > > it's already started. > > On 2014/08/11 20:47:05, nyquist wrote: > > > Could this just be something short like this? > > > > > > document.getElementById('loadingIndicator').className = > > > isLastPage ? 'hidden' : 'visible'; > > > updateLoadingIndicator(isLastPage); > > > > > > Or is the ordering of setting the classname and updating the loading > indicator > > > important (since you had different ordering in if and else branches). I > could > > > see that you might have to "create"/update the loading indicator before > > showing > > > it. > > > > AFAIK the interval only triggers after 600 ms, and not instantly, so I think you > should simplify this. See my previous comment. Done. https://codereview.chromium.org/444143002/diff/170001/components/dom_distille... components/dom_distiller/content/resources/dom_distiller_viewer.js:43: colorIndex = (colorIndex + 1) % colors.length; On 2014/08/12 22:04:32, nyquist wrote: > I think these two lines are missing indentation. Done.
thanks! lgtm
The CQ bit was checked by sunangel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunangel@chromium.org/444143002/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Message was sent while issue was closed.
Committed patchset #6 (190001) as 289601 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
