Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11515005: Delay updating jumplist to avoid blocking the file thread at start-up (Closed)

Created:
8 years ago by Cait (Slow)
Modified:
7 years, 11 months ago
Reviewers:
sky, jeremy
CC:
chromium-reviews, tfarina, jeremy
Visibility:
Public.

Description

Delay updating jumplist to avoid blocking the file thread at start-up BUG=160500 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174450 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174990

Patch Set 1 : #

Total comments: 4

Patch Set 2 : Use NOTIFICATION_LOAD_COMPLETE instead #

Patch Set 3 : clean up a couple nits #

Total comments: 6

Patch Set 4 : #

Total comments: 1

Patch Set 5 : #

Total comments: 2

Patch Set 6 : Add JumpListListener #

Total comments: 24

Patch Set 7 : #

Total comments: 8

Patch Set 8 : #

Total comments: 1

Patch Set 9 : rebase #

Patch Set 10 : rebase and retry #

Patch Set 11 : rebase #

Patch Set 12 : Fix browser_test breakage #

Patch Set 13 : rebase #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -4 lines) Patch
M chrome/browser/jumplist_win.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.h View 1 2 3 4 5 6 7 8 9 4 chunks +10 lines, -1 line 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -2 lines 0 comments Download
A chrome/browser/ui/views/load_complete_listener.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +50 lines, -0 lines 2 comments Download
A chrome/browser/ui/views/load_complete_listener.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +44 lines, -0 lines 4 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
Cait (Slow)
Scott: PTAL -- Thanks!
8 years ago (2012-12-11 14:39:04 UTC) #1
jeremy
Thanks! https://codereview.chromium.org/11515005/diff/1001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/11515005/diff/1001/chrome/browser/jumplist_win.cc#newcode705 chrome/browser/jumplist_win.cc:705: base::TimeDelta::FromSeconds(15)); Some startups are longer than 15 seconds, ...
8 years ago (2012-12-11 14:57:22 UTC) #2
Cait (Slow)
https://chromiumcodereview.appspot.com/11515005/diff/1001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://chromiumcodereview.appspot.com/11515005/diff/1001/chrome/browser/jumplist_win.cc#newcode705 chrome/browser/jumplist_win.cc:705: base::TimeDelta::FromSeconds(15)); On 2012/12/11 14:57:22, jeremy wrote: > Some startups ...
8 years ago (2012-12-11 15:37:44 UTC) #3
jeremy
https://chromiumcodereview.appspot.com/11515005/diff/1001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://chromiumcodereview.appspot.com/11515005/diff/1001/chrome/browser/jumplist_win.cc#newcode705 chrome/browser/jumplist_win.cc:705: base::TimeDelta::FromSeconds(15)); Only for telling when startup is complete, thanks!
8 years ago (2012-12-11 16:50:02 UTC) #4
Cait (Slow)
https://codereview.chromium.org/11515005/diff/1001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/11515005/diff/1001/chrome/browser/jumplist_win.cc#newcode705 chrome/browser/jumplist_win.cc:705: base::TimeDelta::FromSeconds(15)); On 2012/12/11 16:50:02, jeremy wrote: > Only for ...
8 years ago (2012-12-11 20:54:34 UTC) #5
jeremy
https://codereview.chromium.org/11515005/diff/10001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/11515005/diff/10001/chrome/browser/jumplist_win.cc#newcode543 chrome/browser/jumplist_win.cc:543: content::NotificationService::AllSources()); I think 'content::' should be aligned under 'this' ...
8 years ago (2012-12-12 08:16:09 UTC) #6
Cait (Slow)
https://chromiumcodereview.appspot.com/11515005/diff/10001/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://chromiumcodereview.appspot.com/11515005/diff/10001/chrome/browser/jumplist_win.cc#newcode543 chrome/browser/jumplist_win.cc:543: content::NotificationService::AllSources()); On 2012/12/12 08:16:09, jeremy wrote: > I think ...
8 years ago (2012-12-12 17:09:17 UTC) #7
Cait (Slow)
ping jeremy: lgty? On 2012/12/12 17:09:17, caitkp wrote: > https://chromiumcodereview.appspot.com/11515005/diff/10001/chrome/browser/jumplist_win.cc > File chrome/browser/jumplist_win.cc (right): > ...
8 years ago (2012-12-13 21:43:54 UTC) #8
jeremy
LGTM https://codereview.chromium.org/11515005/diff/14002/chrome/browser/jumplist_win.cc File chrome/browser/jumplist_win.cc (right): https://codereview.chromium.org/11515005/diff/14002/chrome/browser/jumplist_win.cc#newcode541 chrome/browser/jumplist_win.cc:541: // Register for notification of when comment still ...
8 years ago (2012-12-13 22:06:24 UTC) #9
Cait (Slow)
sky: for OWNERS approval. thanks! On 2012/12/13 22:06:24, jeremy wrote: > LGTM > > https://codereview.chromium.org/11515005/diff/14002/chrome/browser/jumplist_win.cc ...
8 years ago (2012-12-13 22:18:31 UTC) #10
sky
If we're trying to delay the JumpList from doing anything until the first NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME I ...
8 years ago (2012-12-13 23:07:51 UTC) #11
Cait (Slow)
PTAL -- I added a JumpListListener class to create the jumplist after the first page ...
8 years ago (2012-12-18 17:49:20 UTC) #12
sky
https://codereview.chromium.org/11515005/diff/27001/chrome/browser/jumplist_listener.cc File chrome/browser/jumplist_listener.cc (right): https://codereview.chromium.org/11515005/diff/27001/chrome/browser/jumplist_listener.cc#newcode21 chrome/browser/jumplist_listener.cc:21: Only one newline. https://codereview.chromium.org/11515005/diff/27001/chrome/browser/jumplist_listener.cc#newcode43 chrome/browser/jumplist_listener.cc:43: content::BrowserThread::UI, FROM_HERE, callback_); Why ...
8 years ago (2012-12-18 20:52:38 UTC) #13
Cait (Slow)
https://codereview.chromium.org/11515005/diff/27001/chrome/browser/jumplist_listener.cc File chrome/browser/jumplist_listener.cc (right): https://codereview.chromium.org/11515005/diff/27001/chrome/browser/jumplist_listener.cc#newcode21 chrome/browser/jumplist_listener.cc:21: On 2012/12/18 20:52:38, sky wrote: > Only one newline. ...
8 years ago (2012-12-19 19:27:01 UTC) #14
sky
https://codereview.chromium.org/11515005/diff/35001/chrome/browser/ui/views/load_complete_listener.cc File chrome/browser/ui/views/load_complete_listener.cc (right): https://codereview.chromium.org/11515005/diff/35001/chrome/browser/ui/views/load_complete_listener.cc#newcode39 chrome/browser/ui/views/load_complete_listener.cc:39: if (!called_) { Rather than maintaining called_ can you ...
8 years ago (2012-12-19 22:13:37 UTC) #15
Cait (Slow)
https://codereview.chromium.org/11515005/diff/35001/chrome/browser/ui/views/load_complete_listener.cc File chrome/browser/ui/views/load_complete_listener.cc (right): https://codereview.chromium.org/11515005/diff/35001/chrome/browser/ui/views/load_complete_listener.cc#newcode39 chrome/browser/ui/views/load_complete_listener.cc:39: if (!called_) { On 2012/12/19 22:13:37, sky wrote: > ...
8 years ago (2012-12-19 23:31:38 UTC) #16
sky
LGTM https://codereview.chromium.org/11515005/diff/37001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/11515005/diff/37001/chrome/browser/ui/views/frame/browser_view.cc#newcode2260 chrome/browser/ui/views/frame/browser_view.cc:2260: #if defined(OS_WIN) && !defined(USE_AURA) Don't indent #ifs.
8 years ago (2012-12-20 01:46:28 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/11515005/37002
8 years ago (2012-12-20 15:44:39 UTC) #18
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests
8 years ago (2012-12-20 19:01:10 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/11515005/37002
8 years ago (2012-12-20 19:10:46 UTC) #20
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests
8 years ago (2012-12-20 23:39:20 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/11515005/43001
8 years ago (2012-12-21 15:47:46 UTC) #22
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests
8 years ago (2012-12-21 19:11:35 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/11515005/43001
8 years ago (2012-12-21 20:53:55 UTC) #24
commit-bot: I haz the power
Change committed as 174450
8 years ago (2012-12-21 20:54:45 UTC) #25
oshima
On 2012/12/21 20:54:45, I haz the power (commit-bot) wrote: > Change committed as 174450 This ...
8 years ago (2012-12-22 06:04:33 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/11515005/63002
7 years, 11 months ago (2013-01-03 16:30:51 UTC) #27
commit-bot: I haz the power
Change committed as 174990
7 years, 11 months ago (2013-01-03 19:30:18 UTC) #28
tfarina
7 years, 11 months ago (2013-01-09 23:58:24 UTC) #29
Message was sent while issue was closed. 
I'll fix the things I pointed out below in a follow up CL.

Just fyi! ;)

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
File chrome/browser/ui/views/load_complete_listener.cc (right):

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.cc:7: #include
"chrome/common/chrome_notification_types.h"
this include is not necessary as you already included notification_types.h

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.cc:15: registrar_ = new
content::NotificationRegistrar();
You are leaking this, as far as I can see. You should either allocate this on
stack or use scoped_ptr.

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.cc:24:
content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
indent just 4 spaces after 'if'.

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.cc:32: const
content::NotificationSource& source,
wrong indentation here.

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
File chrome/browser/ui/views/load_complete_listener.h (right):

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.h:8: #include
"content/public/browser/browser_thread.h"
you don't use this include here.

https://chromiumcodereview.appspot.com/11515005/diff/63002/chrome/browser/ui/...
chrome/browser/ui/views/load_complete_listener.h:12: class NotificationSource;
you don't need to forward declare this and Details because they come from
notification_observer.h

Powered by Google App Engine
This is Rietveld 408576698