Disable caret blinking for blimp (issue 2381373002 by shaktisahu@chromium.org)

1 view
Skip to first unread message

shakt...@chromium.org

unread,
Oct 1, 2016, 1:21:55 AM10/1/16
to dtrainor...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
Reviewers: David Trainor
CL: https://codereview.chromium.org/2381373002/

Description:
Disable caret blinking for blimp

For blimp, blinking caret in a text input area causes lot of redundant
state update messages being sent between client and server. To reduce
the data usage, a settings was added to blink to disable the blinking.

BUG=

Affected files (+17, -0 lines):
M blimp/engine/app/settings_manager.cc
M content/public/common/common_param_traits_macros.h
M content/public/common/web_preferences.h
M content/public/common/web_preferences.cc
M content/renderer/render_view_impl.cc
M third_party/WebKit/Source/core/editing/FrameCaret.cpp
M third_party/WebKit/Source/core/frame/Settings.in
M third_party/WebKit/Source/web/WebSettingsImpl.h
M third_party/WebKit/Source/web/WebSettingsImpl.cpp
M third_party/WebKit/public/web/WebSettings.h


Index: blimp/engine/app/settings_manager.cc
diff --git a/blimp/engine/app/settings_manager.cc b/blimp/engine/app/settings_manager.cc
index 0c7fd35b62156cfcff0a9827154264278670932d..46163439089efc1b2aef7f59300b133da2243886 100644
--- a/blimp/engine/app/settings_manager.cc
+++ b/blimp/engine/app/settings_manager.cc
@@ -31,6 +31,7 @@ void SettingsManager::UpdateWebkitPreferences(content::WebPreferences* prefs) {
settings_.default_maximum_page_scale_factor;
prefs->shrinks_viewport_contents_to_fit =
settings_.shrinks_viewport_contents_to_fit;
+ prefs->disable_caret_blinking = true;
}

const EngineSettings& SettingsManager::GetEngineSettings() const {
Index: content/public/common/common_param_traits_macros.h
diff --git a/content/public/common/common_param_traits_macros.h b/content/public/common/common_param_traits_macros.h
index ea3dc292ae23b114434e04d36c0a719517679c0c..5ef1c6a1fb62ed4b08fe633268aa3d49a2e43d78 100644
--- a/content/public/common/common_param_traits_macros.h
+++ b/content/public/common/common_param_traits_macros.h
@@ -149,6 +149,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::WebPreferences)
IPC_STRUCT_TRAITS_MEMBER(hyperlink_auditing_enabled)
IPC_STRUCT_TRAITS_MEMBER(allow_universal_access_from_file_urls)
IPC_STRUCT_TRAITS_MEMBER(allow_file_access_from_file_urls)
+ IPC_STRUCT_TRAITS_MEMBER(disable_caret_blinking)
IPC_STRUCT_TRAITS_MEMBER(experimental_webgl_enabled)
IPC_STRUCT_TRAITS_MEMBER(pepper_3d_enabled)
IPC_STRUCT_TRAITS_MEMBER(inert_visual_viewport)
Index: content/public/common/web_preferences.cc
diff --git a/content/public/common/web_preferences.cc b/content/public/common/web_preferences.cc
index 96b7a649b28af0dd63ad91a15c6967a369ec6142..6b361903f8d7f2a88506ef243eee2af59ceecd88 100644
--- a/content/public/common/web_preferences.cc
+++ b/content/public/common/web_preferences.cc
@@ -99,6 +99,7 @@ WebPreferences::WebPreferences()
hyperlink_auditing_enabled(true),
allow_universal_access_from_file_urls(false),
allow_file_access_from_file_urls(false),
+ disable_caret_blinking(false),
experimental_webgl_enabled(false),
pepper_3d_enabled(false),
flash_3d_enabled(true),
Index: content/public/common/web_preferences.h
diff --git a/content/public/common/web_preferences.h b/content/public/common/web_preferences.h
index d31bf84c9212385cb14640344fa51e361a7ab02d..724ef1c8405774b90f9c3ebe1e1622c25e885cca 100644
--- a/content/public/common/web_preferences.h
+++ b/content/public/common/web_preferences.h
@@ -120,6 +120,7 @@ struct CONTENT_EXPORT WebPreferences {
bool hyperlink_auditing_enabled;
bool allow_universal_access_from_file_urls;
bool allow_file_access_from_file_urls;
+ bool disable_caret_blinking;
bool experimental_webgl_enabled;
bool pepper_3d_enabled;
bool flash_3d_enabled;
Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index c31ba7c346ac09949f33c29a95c8fd22ce7ba8a4..a3197caf908158e21d4f6216c8332acd14c1c222 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -988,6 +988,8 @@ void RenderView::ApplyWebPreferences(const WebPreferences& prefs,
settings->setAllowFileAccessFromFileURLs(
prefs.allow_file_access_from_file_urls);

+ settings->setDisableCaretBlinking(prefs.disable_caret_blinking);
+
// Enable experimental WebGL support if requested on command line
// and support is compiled in.
settings->setExperimentalWebGLEnabled(prefs.experimental_webgl_enabled);
Index: third_party/WebKit/Source/core/editing/FrameCaret.cpp
diff --git a/third_party/WebKit/Source/core/editing/FrameCaret.cpp b/third_party/WebKit/Source/core/editing/FrameCaret.cpp
index 78a623abfa1d66a43ef88c838cd7b7193f60c91b..97a6b46fd53dfd5280fae8b44cbdbfd0a70e6f9a 100644
--- a/third_party/WebKit/Source/core/editing/FrameCaret.cpp
+++ b/third_party/WebKit/Source/core/editing/FrameCaret.cpp
@@ -260,6 +260,9 @@ bool FrameCaret::shouldBlinkCaret() const
if (m_frame->settings() && m_frame->settings()->caretBrowsingEnabled())
return false;

+ if (m_frame->settings() && m_frame->settings()->disableCaretBlinking())
+ return false;
+
Element* root = rootEditableElementOf(caretPosition().position());
if (!root)
return false;
Index: third_party/WebKit/Source/core/frame/Settings.in
diff --git a/third_party/WebKit/Source/core/frame/Settings.in b/third_party/WebKit/Source/core/frame/Settings.in
index 358dfe5bebe1c07c586aae761c187753d0f07c61..7e096facb1317d0b6a1ad0cfac3ec2880bd99167 100644
--- a/third_party/WebKit/Source/core/frame/Settings.in
+++ b/third_party/WebKit/Source/core/frame/Settings.in
@@ -61,6 +61,7 @@ caretBrowsingEnabled initial=false
localStorageEnabled initial=false
allowUniversalAccessFromFileURLs initial=true
allowFileAccessFromFileURLs initial=true
+disableCaretBlinking initial=false
javaScriptCanOpenWindowsAutomatically initial=false
supportsMultipleWindows initial=true
javaScriptCanAccessClipboard initial=false
Index: third_party/WebKit/Source/web/WebSettingsImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebSettingsImpl.cpp b/third_party/WebKit/Source/web/WebSettingsImpl.cpp
index adac1a3e85996e39feeeae485c5c6151be3aaab3..e4ad838fc76d3971f015456d90266e274df0a121 100644
--- a/third_party/WebKit/Source/web/WebSettingsImpl.cpp
+++ b/third_party/WebKit/Source/web/WebSettingsImpl.cpp
@@ -462,6 +462,11 @@ void WebSettingsImpl::setAllowUniversalAccessFromFileURLs(bool allow)
m_settings->setAllowUniversalAccessFromFileURLs(allow);
}

+void WebSettingsImpl::setDisableCaretBlinking(bool disableCaretBlinking)
+{
+ m_settings->setDisableCaretBlinking(disableCaretBlinking);
+}
+
void WebSettingsImpl::setAllowFileAccessFromFileURLs(bool allow)
{
m_settings->setAllowFileAccessFromFileURLs(allow);
Index: third_party/WebKit/Source/web/WebSettingsImpl.h
diff --git a/third_party/WebKit/Source/web/WebSettingsImpl.h b/third_party/WebKit/Source/web/WebSettingsImpl.h
index 00b3dc6ec56107acd455f67946e8ac5601792bc6..105c17dd531598faaaecd2c51ce2d8eb8ae418bb 100644
--- a/third_party/WebKit/Source/web/WebSettingsImpl.h
+++ b/third_party/WebKit/Source/web/WebSettingsImpl.h
@@ -56,6 +56,7 @@ public:
void setAccessibilityEnabled(bool) override;
void setAccessibilityPasswordValuesEnabled(bool) override;
void setAllowFileAccessFromFileURLs(bool) override;
+ void setDisableCaretBlinking(bool) override;
void setAllowCustomScrollbarInMainFrame(bool) override;
void setAllowGeolocationOnInsecureOrigins(bool) override;
void setAllowRunningOfInsecureContent(bool) override;
Index: third_party/WebKit/public/web/WebSettings.h
diff --git a/third_party/WebKit/public/web/WebSettings.h b/third_party/WebKit/public/web/WebSettings.h
index 3ba8df2fb1560f7684fac63de2a3c86f7ea6832e..c9c9cdbdca4e4bf2cd32c847b354c0da0073dea3 100644
--- a/third_party/WebKit/public/web/WebSettings.h
+++ b/third_party/WebKit/public/web/WebSettings.h
@@ -129,6 +129,7 @@ public:
virtual void setAccessibilityEnabled(bool) = 0;
virtual void setAccessibilityPasswordValuesEnabled(bool) = 0;
virtual void setAllowFileAccessFromFileURLs(bool) = 0;
+ virtual void setDisableCaretBlinking(bool) = 0;
virtual void setAllowCustomScrollbarInMainFrame(bool) = 0;
virtual void setAllowGeolocationOnInsecureOrigins(bool) = 0;
// If set to true, allows frames with an https origin to run active


nyq...@chromium.org

unread,
Oct 5, 2016, 12:02:42 AM10/5/16
to shakt...@chromium.org, dtrainor...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
Also nit; could you swap 'server' for 'engine' in the CL description?


https://codereview.chromium.org/2381373002/diff/1/content/renderer/render_view_impl.cc
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2381373002/diff/1/content/renderer/render_view_impl.cc#newcode991
content/renderer/render_view_impl.cc:991:
settings->setDisableCaretBlinking(prefs.disable_caret_blinking);
Could you add a comment as to what this does?

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/public/web/WebSettings.h
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/public/web/WebSettings.h#newcode132
third_party/WebKit/public/web/WebSettings.h:132: virtual void
setDisableCaretBlinking(bool) = 0;
It looks like these are supposed to be alphabetically sorted.

https://codereview.chromium.org/2381373002/

bgol...@chromium.org

unread,
Oct 5, 2016, 12:46:46 PM10/5/16
to shakt...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
Can you add a TEST= line to the description?

https://codereview.chromium.org/2381373002/

shakt...@chromium.org

unread,
Oct 5, 2016, 1:07:32 PM10/5/16
to dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, a...@chromium.org, s...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
avi@ - PTAL at content/

sky@ - PTAL at third_party/WebKit/



https://codereview.chromium.org/2381373002/

a...@chromium.org

unread,
Oct 5, 2016, 1:44:27 PM10/5/16
to shakt...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, s...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
content lgtm

You will need a content reviewer.

https://codereview.chromium.org/2381373002/

a...@chromium.org

unread,
Oct 5, 2016, 1:45:15 PM10/5/16
to shakt...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, s...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
On 2016/10/05 17:44:26, Avi wrote:
> content lgtm
>
> You will need a content reviewer.

Sorry, you will need an IPC reviewer.

https://codereview.chromium.org/2381373002/

s...@chromium.org

unread,
Oct 6, 2016, 11:46:43 AM10/6/16
to shakt...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, a...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
On 2016/10/05 17:07:31, shaktisahu wrote:
> avi@ - PTAL at content/
>
> sky@ - PTAL at third_party/WebKit/

I'm not a good third_party/WebKit owner. Please file a local owner.

https://codereview.chromium.org/2381373002/

s...@chromium.org

unread,
Oct 6, 2016, 11:46:59 AM10/6/16
to shakt...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, a...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org

dtra...@chromium.org

unread,
Oct 6, 2016, 12:38:55 PM10/6/16
to shakt...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, a...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
lgtm % nits to move some methods around.


https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/Source/web/WebSettingsImpl.cpp
File third_party/WebKit/Source/web/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/Source/web/WebSettingsImpl.cpp#newcode465
third_party/WebKit/Source/web/WebSettingsImpl.cpp:465: void
WebSettingsImpl::setDisableCaretBlinking(bool disableCaretBlinking)
Move to above setDisableReadingFromCanvas

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/Source/web/WebSettingsImpl.h
File third_party/WebKit/Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/Source/web/WebSettingsImpl.h#newcode59
third_party/WebKit/Source/web/WebSettingsImpl.h:59: void
setDisableCaretBlinking(bool) override;
Move this down to above setDisableReadingFromCanvas?

https://codereview.chromium.org/2381373002/

shakt...@chromium.org

unread,
Oct 6, 2016, 4:43:23 PM10/6/16
to dtrainor...@chromium.org, nyquist+...@chromium.org, bgol...@chromium.org, a...@chromium.org, dgla...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
dglazkov@ - PTAL at third_party/WebKit/



https://codereview.chromium.org/2381373002/

nyq...@chromium.org

unread,
Oct 6, 2016, 7:15:24 PM10/6/16
to shakt...@chromium.org, dtrainor...@chromium.org, bgol...@chromium.org, a...@chromium.org, dgla...@chromium.org, chromium...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, dglazko...@chromium.org, kmarshall+...@chromium.org, kinuko...@chromium.org, shaktisahu+...@chromium.org, maniscalco+...@chromium.org, bgoldman+w...@chromium.org, j...@chromium.org, gcasto+wa...@chromium.org, marcinjb+w...@chromium.org, jessicag+w...@chromium.org, dari...@chromium.org, lethalantidot...@chromium.org, blink-...@chromium.org, scf+wat...@chromium.org, mlamouri+wa...@chromium.org, creis...@chromium.org, khushalsagar...@chromium.org, anandc+wa...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org, blink-re...@chromium.org, perumaal+w...@chromium.org, dtrainor+w...@chromium.org
lgtm, but please add a TEST= like bgoldman@ suggested.

https://codereview.chromium.org/2381373002/

esp...@chromium.org

unread,
Oct 7, 2016, 1:42:37 PM10/7/16
to shakt...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
I think you want to instead move the caret to be a separate composited layer and
have the blinking be done client side like selection handles. Many users have
never seen one before and I susoefits confusing and looks like the page is
stuck.

Separately we have a setting to control the blink speed of the caret already.
Doing WebRenderTheme::setCaretBlinkInterval(0) should already disable the
blinking.

Not lgtm :)

https://codereview.chromium.org/2381373002/

esp...@chromium.org

unread,
Oct 7, 2016, 1:43:21 PM10/7/16
to shakt...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
On 2016/10/07 at 17:42:36, esprehn wrote:
> I think you want to instead move the caret to be a separate composited layer
and have the blinking be done client side like selection handles. Many users
have never seen one before and I susoefits

dtra...@chromium.org

unread,
Oct 7, 2016, 1:57:29 PM10/7/16
to shakt...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, nyquist+...@chromium.org, esp...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
On 2016/10/07 17:43:20, esprehn wrote:
> On 2016/10/07 at 17:42:36, esprehn wrote:
> > I think you want to instead move the caret to be a separate composited layer
> and have the blinking be done client side like selection handles. Many users
> have never seen one before and I susoefits
> ^ suspect*

We'll have a modal dialog instead of doing text insertion directly into the page
content (ping me on chat if you want to see the mocks), so the page content
won't be visible or interactive. We just don't want blink doing unnecessary
work if we know it's not needed.

That's a good idea about using the renderer preferences :).

https://codereview.chromium.org/2381373002/

shakt...@chromium.org

unread,
Oct 7, 2016, 8:32:13 PM10/7/16
to a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, esp...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
Much simpler now :).
Using caret_blink_interval = 0 instead.
dtrainor@, esprehn@ - PTAL.

https://codereview.chromium.org/2381373002/

esp...@chromium.org

unread,
Oct 7, 2016, 8:40:56 PM10/7/16
to shakt...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org

khusha...@chromium.org

unread,
Oct 10, 2016, 2:57:10 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
lgtm.


https://codereview.chromium.org/2381373002/diff/40001/blimp/engine/session/tab.cc
File blimp/engine/session/tab.cc (right):

https://codereview.chromium.org/2381373002/diff/40001/blimp/engine/session/tab.cc#newcode108
blimp/engine/session/tab.cc:108: render_view_host->SyncRendererPrefs();
Would it be nicer if content gave a callback before the Prefs are sent
to the renderer, something along the lines of:
https://cs.chromium.org/chromium/src/content/public/browser/content_browser_client.h?sq=package:chromium&rcl=1476098929&l=536

https://codereview.chromium.org/2381373002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 10, 2016, 2:59:41 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, commi...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 10, 2016, 3:04:02 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, commi...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
Try jobs failed on following builders:
chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-generic_chromium_compile_only_ng/builds/213830)
linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/240732)
linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_clobber_rel_ng/builds/250630)
linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/313165)

https://codereview.chromium.org/2381373002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 10, 2016, 3:30:10 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, commi...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
CQ is trying da patch. Follow status at

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 10, 2016, 4:28:34 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, commi...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/2381373002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 10, 2016, 4:30:15 PM10/10/16
to shakt...@chromium.org, esp...@chromium.org, a...@chromium.org, bgol...@chromium.org, dgla...@chromium.org, dtrainor...@chromium.org, nyquist+...@chromium.org, commi...@chromium.org, anandc+wa...@chromium.org, bgoldman+w...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, creis...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, dtrainor+w...@chromium.org, gcasto+wa...@chromium.org, j...@chromium.org, jessicag+w...@chromium.org, khushalsagar...@chromium.org, kinuko...@chromium.org, kmarshall+...@chromium.org, lethalantidot...@chromium.org, maniscalco+...@chromium.org, marcinjb+w...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, nyquist+w...@chromium.org, perumaal+w...@chromium.org, scf+wat...@chromium.org, shaktisahu+...@chromium.org, sriramsr+w...@chromium.org, steimel+w...@chromium.org
Patchset 4 (id:??) landed as
https://crrev.com/22c4d40260078dfe23612368f2ae9519729062ce
Cr-Commit-Position: refs/heads/master@{#424222}

https://codereview.chromium.org/2381373002/
Reply all
Reply to author
Forward
0 new messages