[webkit-reviews] review granted: [Bug 101274] Allow plugins to be disabled by shared library filename : [Attachment 179076] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has granted Parth Patel
<parpatel at rim.com>'s request for review:
Bug 101274: Allow plugins to be disabled by shared library filename
https://bugs.webkit.org/show_bug.cgi?id=101274

Attachment 179076: Patch
https://bugs.webkit.org/attachment.cgi?id=179076&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179076&action=review


Looks good.  Please address the comments below and any concern Yong might have.


Next, reupload the patch with "Reviewed by Antonio Gomes"  in the ChangeLog,
and only request commit-queue to land it. No further review needed.

> Source/WebKit/blackberry/Api/WebPage.cpp:5158
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

::installedPlugins(true /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5161
> +	   return;
> +    // Reload

replace this "reload" comment by an empty line.

> Source/WebKit/blackberry/Api/WebPage.cpp:5178
> +    d->m_page->refreshPlugins(false);

refreshPlugins(false /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5187
> +    PluginDatabase* database = PluginDatabase::installedPlugins(true);

::installedPlugins(true /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5191
> +    if (!(disabled ? database->addDisabledPluginFile(fileName) :
database->removeDisabledPluginFile(fileName)))

This line does not read well to me. Use a help variable, please.

> Source/WebKit/blackberry/Api/WebPage.cpp:5200
> +    d->m_page->refreshPlugins(false);

refreshPlugins(false /*whatDoesTheBoolMeanHerePlease*/);

> Source/WebKit/blackberry/Api/WebPage.cpp:5202
> +    // Refresh the plugin database if necessary

Add a dot at the end of the sentence.