Issue 3034033: Our InPlaceMenu implementation is only partially being used....

Keyboard Shortcuts

	File
u :	up to issue
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line

	Issue
u :	up to list of issues
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue

Issue 3034033: Our InPlaceMenu implementation is only partially being used.... (Closed)

Created:
10 years, 5 months ago by tommi (sloooow) - chröme

Modified:
9 years, 7 months ago

Reviewers:
ananta

CC:
chromium-reviews, amit

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

Our InPlaceMenu implementation is only partially being used. We had a leftover call to InPlaceMenuDestroy without creating a menu in the first place. This caused us to delete IE's menu and fail the first check inside ieframe!CDocObjectHost::OnInitMenuPopup. It took a while to figure this out and even longer to figure out why our menu was NULL in this case and not mshtml's. I don't think that there are any bad side effects with this change but I ask the reviewer to check. The visible change is that the Print menu item will become enabled and visible whereas Page Setup will be disabled and Print Preview will (correctly atm) not be visible for GCF documents. BUG=24034, 47074 TEST=See description above and bug report. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53786

Patch Set 1 #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+5 lines, -1 line)			Patch
	M	chrome_frame/ole_document_impl.h	View		1 chunk	+5 lines, -1 line	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

amit

10 years, 5 months ago (2010-07-26 21:13:06 UTC) #3

Awesome find! :)

On Mon, Jul 26, 2010 at 1:54 PM,  <ananta@chromium.org> wrote:
> LGTM++ Thanks for fixing this.
>
>
> http://codereview.chromium.org/3034033/show
>

Expand Messages | Collapse Messages