[Webkit-unassigned] [Bug 63857] [DRT] implement createPopupMenu for Chromium DRT port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63857


Johnny(Jianning) Ding <jnd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99880|                            |review?
               Flag|                            |




--- Comment #7 from Johnny(Jianning) Ding <jnd at chromium.org>  2011-07-06 14:23:14 PST ---
Created an attachment (id=99880)
 --> (https://bugs.webkit.org/attachment.cgi?id=99880&action=review)
patch v2

> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:249
> > +        if (widget)
> 
> Nit: Do we really need this check?  Shouldn't WebPopupMenu::create always succeed?

If not considering OOM situation, we need not to check the pointer. I will drop it since WebKit will eventually crash if OOM.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:254
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> I would remove the default case.  That way if someone adds another WebPopupType, there will be a compiler error and the person can update this code.

done.

> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:261
> > +    // In Chromium, we do not this method.
> 
> Nit: This comment isn't that useful.  Can you either explain why we don't need this method or just remove the comment?

done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.