New issue
Advanced search Search tips

Issue 887792 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Remove the addPageLoadHandler helper.

Project Member Reported by noel@chromium.org, Sep 21

Issue description

Follow on from  issue 884963 , DOMContentLoaded use.  Review remaining files using the helper util.addPageLoadHandler(), change them to use DOMContentLoaded directly, and remove the addPageLoadHandler helper.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a0eff86adda59cbe455149e3ea70644fe633cc4

commit 5a0eff86adda59cbe455149e3ea70644fe633cc4
Author: Noel Gordon <noel@chromium.org>
Date: Fri Sep 21 03:33:10 2018

Remove addPageLoadHandler use from file manager main.js

Call DOMContentLoaded directly. Update comments.

Bug:  887792 
Change-Id: Ib688e8dda1b74ff065e91a980099050ca19cb8a4
Reviewed-on: https://chromium-review.googlesource.com/1237018
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593068}
[modify] https://crrev.com/5a0eff86adda59cbe455149e3ea70644fe633cc4/ui/file_manager/file_manager/foreground/js/main.js

Cc: lucmult@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6829537632e6e7c721b6c76b0296120b0f32b57d

commit 6829537632e6e7c721b6c76b0296120b0f32b57d
Author: Noel Gordon <noel@chromium.org>
Date: Fri Sep 21 04:38:36 2018

Remove unused file manager variable pageLoaded

This variable was added in [1] but the patch never used the variable,
remove it.

[1] https://goo.gl/Fnvppi (good read btw, explaining why file manager
has many separate initialization steps).

Bug:  887792 
Change-Id: Ic9af3447382589bf27fc84fe7a6613c2a269d75d
Reviewed-on: https://chromium-review.googlesource.com/1237894
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593078}
[modify] https://crrev.com/6829537632e6e7c721b6c76b0296120b0f32b57d/ui/file_manager/file_manager/foreground/js/main.js

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7aca39ac40888016756b8ca46bfc9d244466133

commit f7aca39ac40888016756b8ca46bfc9d244466133
Author: Noel Gordon <noel@chromium.org>
Date: Fri Sep 21 08:06:45 2018

File manager main: use readyState "loading" DOMContentLoaded

main_scripts.js has a defer attribute: add DOMContentLoaded if needed,
ie., only if the document is loading. And modernize the remaining code
while here.

Bug:  887792 
Change-Id: I04b9d346025d0f9a607772c2a340d80573489079
Reviewed-on: https://chromium-review.googlesource.com/1237801
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593111}
[modify] https://crrev.com/f7aca39ac40888016756b8ca46bfc9d244466133/ui/file_manager/file_manager/foreground/js/main.js

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebd9b1a8c901223b7d74aec49397abdbfa081b87

commit ebd9b1a8c901223b7d74aec49397abdbfa081b87
Author: Noel Gordon <noel@chromium.org>
Date: Mon Sep 24 00:37:21 2018

Remove addPageLoadHandler use from video player caster.js

Install a DOMContentLoaded handler if needed.

Bug:  887792 
Change-Id: I19322c024829cf894cb21799c90b707ce1e37826
Reviewed-on: https://chromium-review.googlesource.com/1237837
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593462}
[modify] https://crrev.com/ebd9b1a8c901223b7d74aec49397abdbfa081b87/ui/file_manager/video_player/js/cast/caster.js

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73a4454ca40545520b433d9dc8daa0909a7b04fd

commit 73a4454ca40545520b433d9dc8daa0909a7b04fd
Author: Noel Gordon <noel@chromium.org>
Date: Mon Sep 24 01:56:24 2018

Remove unused util.addPageLoadHandler

Bug:  887792 
Change-Id: I92c9de0f5af446ece415df03a537dd9a2c5de8ac
Reviewed-on: https://chromium-review.googlesource.com/1238296
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593470}
[modify] https://crrev.com/73a4454ca40545520b433d9dc8daa0909a7b04fd/ui/file_manager/file_manager/common/js/util.js

Status: Fixed (was: Started)
Status: Started (was: Fixed)
Re-opening to recover util.testSendMessage('ready') at Files app startup.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3442ee04f80ead42e7730aabc84250e5e2bdfa9e

commit 3442ee04f80ead42e7730aabc84250e5e2bdfa9e
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 16 02:48:01 2018

Send util.testSendMessage('ready') at Files app start-up

CL:1237801 removed this start-up message, but it was being used in the
ui/views/select_file_dialog_extension_browsertest.cc tests. These file
extension dialog browser tests didn't complain about the message being
removed because the they're DISABLED on Chrome OS ¯\(ツ)/¯

We should get those test running again, which will be the subject of a
of a new testing bug. For now, recover the 'ready' start-up message.

Bug:  453634 ,  887792 
Change-Id: I69b5118f9ebab6d7ee2ac2285ac714b223a437a4
Reviewed-on: https://chromium-review.googlesource.com/c/1282482
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599825}
[modify] https://crrev.com/3442ee04f80ead42e7730aabc84250e5e2bdfa9e/ui/file_manager/file_manager/foreground/js/main.js

Status: Fixed (was: Started)

Sign in to add a comment