apimall / chromiumembedded

Automatically exported from code.google.com/p/chromiumembedded
0 stars 1 forks source link

Add ability to differentiate between automatic and user-initiated popups #1525

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Add a |user_guesture| parameter to CefLifeSpanHandler::OnBeforePopup. 
OnBeforePopup is called from CefContentBrowserClient::CanCreateWindow. Chromium 
passes a |user_guesture| parameter which indicates whether the popup opened 
automatically (e.g. via the DomContentLoaded event) or via explicit user 
intervention (e.g. the user clicked a button). Applications might wish to block 
the first popup while allowing the second popup.

Original issue reported on code.google.com by magreenb...@gmail.com on 5 Feb 2015 at 11:01

GoogleCodeExporter commented 9 years ago
Issue 1560 has been merged into this issue.

Original comment by magreenb...@gmail.com on 3 Mar 2015 at 5:25

GoogleCodeExporter commented 9 years ago
Thanks for the patch (from issue #1560). Some comments:

1. Line 6:

                              CefWindowInfo& windowInfo,
                              CefRefPtr<CefClient>& client,
                              CefBrowserSettings& settings,
+                             CefWindowOpenDisposition target_disposition,
+                             bool user_gesture,
                              bool* no_javascript_access) {

Update the method documentation to describe what these new parameters mean. 
Also, put the new input parameters before the output parameters (windowInfo, 
etc).

2. Line 47:

+// Possible targets for a clicked link

Use correct punctuation (add a period).

+///
+typedef enum {
+  TARGET_UNKNOWN = 0,

Enum values should have a more distinct prefix to avoid conflicts with other 
libraries (e.g. "WOD_" to match the enum name -- Googling for "TARGET_UNKNOWN 
define" shows a lot of potential conflicts). Also, when the assignment is 
sequential there's no need to explicitly specify the numeric values.

+  TARGET_SUPPRESS_OPEN = 1,
+  TARGET_CURRENT_TAB = 2,
+  TARGET_SINGLETON_TAB = 3,
+  TARGET_NEW_FOREGROUND_TAB = 4,
+  TARGET_NEW_BACKGROUND_TAB = 5,
+  TARGET_NEW_POPUP = 6,
+  TARGET_NEW_WINDOW = 7,
+  TARGET_SAVE_TO_DISK = 8,
+  TARGET_OFF_THE_RECORD = 9,
+  TARGET_IGNORE_ACTION = 10,
+  TARGET_WINDOW_OPEN_DISPOSITION_LAST = 10

We don't need the WINDOW_OPEN_DISPOSITION_LAST value. That's an implementation 
detail of Chromium.

+} cef_window_open_disposition_t;
+typedef cef_window_open_disposition_t CefWindowOpenDisposition;

CamelCaps definitions are not added in cef_types.h. They should go in the C++ 
header instead.

3. Line 102:

+         static_cast<CefWindowOpenDisposition>(disposition),
+          user_gesture,

Use spaces instead of tabs.

4. Wrap lines at 80 characters.

5. Create patch files using the `git diff --no-prefix` command (this removes 
the a/ b/ prefixes from the file paths).

6. Update unit tests.

Added in trunk revision 2053 with the above changes.

Original comment by magreenb...@gmail.com on 6 Mar 2015 at 9:40