Issue 11572031: "Open All Bookmarks in Incognito Window" opens only valid URLs

Issue 11572031: "Open All Bookmarks in Incognito Window" opens only valid URLs (Closed)

Created:
8 years ago by yoichio

Modified:
8 years ago

Reviewers:
sky

CC:
chromium-reviews, tfarina, yosin_UTC9

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

"Open All Bookmarks in Incognito Window" opens only valid URLs except it can't be opened in incognito window. BUG=106607 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173422

Patch Set 3 : add unittest and codes for mac and gtk #

Total comments: 2

Patch Set 4 : modify mac build failure #

Total comments: 2

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+53 lines, -21 lines)			Patch
M	chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/bookmarks/bookmark_utils.h	View	2 3	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/bookmarks/bookmark_utils.cc	View	1 2 3	4 chunks	+19 lines, -6 lines	1 comment	Download
M	chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm	View	1 2 3	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/gtk/bookmarks/bookmark_bar_gtk.cc	View	1 2	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/gtk/bookmarks/bookmark_menu_controller_gtk.cc	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views_win.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc	View	1 2	3 chunks	+14 lines, -2 lines	1 comment	Download
M	chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

yoichio

This issue includes code for Windows, Mac and Linux. sky, could you review this cl?

8 years ago (2012-12-14 06:51:18 UTC) #2

sky

LGTM https://codereview.chromium.org/11572031/diff/4001/chrome/browser/ui/bookmarks/bookmark_utils.cc File chrome/browser/ui/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/11572031/diff/4001/chrome/browser/ui/bookmarks/bookmark_utils.cc#newcode68 chrome/browser/ui/bookmarks/bookmark_utils.cc:68: if (initial_disposition == OFF_THE_RECORD && nit: you have ...

8 years ago (2012-12-14 17:41:12 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/11572031/13001

8 years ago (2012-12-17 04:27:30 UTC) #4

tfarina

8 years ago (2012-12-19 15:55:03 UTC) #6

Message was sent while issue was closed.

https://chromiumcodereview.appspot.com/11572031/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/bookmarks/bookmark_utils.cc (right):

https://chromiumcodereview.appspot.com/11572031/diff/13001/chrome/browser/ui/...
chrome/browser/ui/bookmarks/bookmark_utils.cc:62: void OpenAllImpl(const
BookmarkNode* node,
ideally I think we should just blow away this function entirely and come up with
a new function that does the RIGHT thing! This function has caused many problems
before and many attempts have been made to fix it, without success. IMO we
should rethink its algorithm/idea.

https://chromiumcodereview.appspot.com/11572031/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc (right):

https://chromiumcodereview.appspot.com/11572031/diff/13001/chrome/browser/ui/...
chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc:141:
ASSERT_EQ(static_cast<size_t>(2), navigator_.urls_.size());
s/static_cast<size_t>(2)/2U

Expand Messages | Collapse Messages