[webkit-reviews] review requested: [Bug 63857] [DRT] implement createPopupMenu for Chromium DRT port : [Attachment 99880] patch v2

[bugzilla-daemon at webkit.org]

Johnny(Jianning) Ding <jnd at chromium.org> has asked  for review:
Bug 63857: [DRT] implement createPopupMenu for Chromium DRT port
https://bugs.webkit.org/show_bug.cgi?id=63857

Attachment 99880: patch v2
https://bugs.webkit.org/attachment.cgi?id=99880&action=review

------- Additional Comments from Johnny(Jianning) Ding <jnd at chromium.org>
> > 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.