Disable caret blinking for blimp

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=652899
Committed: https://crrev.com/22c4d40260078dfe23612368f2ae9519729062ce
Cr-Commit-Position: refs/heads/master@{#424222}

[shaktisahu, 2016-10-01 05:21:54]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[shaktisahu, 2016-10-04 23:17:03]

Description was changed from

==========
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=
==========

to

==========
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=
==========

[shaktisahu, 2016-10-04 23:17:03]

shaktisahu@chromium.org changed reviewers:
+ nyquist@chromium.org

[shaktisahu, 2016-10-04 23:45:28]

Description was changed from

==========
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=
==========

to

==========
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=652899
==========

[nyquist, 2016-10-05 04:02:42]

Also nit; could you swap 'server' for 'engine' in the CL description?

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

https://codereview.chromium.org/2381373002/diff/1/content/renderer/render_vie...
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/w...
File third_party/WebKit/public/web/WebSettings.h (right):

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

[Brian Goldman, 2016-10-05 16:46:45]

bgoldman@chromium.org changed reviewers:
+ bgoldman@chromium.org

[Brian Goldman, 2016-10-05 16:46:46]

Can you add a TEST= line to the description?

[shaktisahu, 2016-10-05 17:07:31]

shaktisahu@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org

[shaktisahu, 2016-10-05 17:07:31]

avi@ - PTAL at content/ 

sky@ - PTAL at third_party/WebKit/

[Avi (use Gerrit), 2016-10-05 17:44:26]

content lgtm

You will need a content reviewer.

[Avi (use Gerrit), 2016-10-05 17:44:44]

On 2016/10/05 17:44:26, Avi wrote:
> content lgtm
> 
> You will need a content reviewer.

Sorry, you will need an IPC reviewer.

[sky, 2016-10-06 15:46:43]

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.

[sky, 2016-10-06 15:47:03]

sky@chromium.org changed reviewers:
- sky@chromium.org

[David Trainor- moved to gerrit, 2016-10-06 16:38:55]

lgtm % nits to move some methods around.

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

https://codereview.chromium.org/2381373002/diff/1/third_party/WebKit/Source/w...
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/w...
File third_party/WebKit/Source/web/WebSettingsImpl.h (right):

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

[shaktisahu, 2016-10-06 20:43:20]

shaktisahu@chromium.org changed reviewers:
+ dglazkov@chromium.org

[shaktisahu, 2016-10-06 20:43:21]

dglazkov@ - PTAL at third_party/WebKit/

[nyquist, 2016-10-06 23:15:23]

lgtm, but please add a TEST= like bgoldman@ suggested.

[esprehn, 2016-10-07 17:42:35]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-10-07 17:42:36]

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 :)

[esprehn, 2016-10-07 17:43:20]

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*

[David Trainor- moved to gerrit, 2016-10-07 17:57:28]

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 :).

[shaktisahu, 2016-10-08 00:32:12]

Much simpler now :).
Using caret_blink_interval = 0 instead.
dtrainor@, esprehn@ - PTAL.

[esprehn, 2016-10-08 00:40:55]

lgtm

[Khushal, 2016-10-10 18:57:09]

lgtm.

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

https://codereview.chromium.org/2381373002/diff/40001/blimp/engine/session/ta...
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_c...

[shaktisahu, 2016-10-10 18:59:16]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-10-10 18:59:17]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dtrainor@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2381373002/#ps40001
(title: "Using RendererPrefereces")

[commit-bot: I haz the power, 2016-10-10 18:59:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 19:04:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-10 19:04:01]

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-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-10-10 19:29:38]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-10-10 19:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
esprehn@chromium.org, dtrainor@chromium.org, nyquist@chromium.org,
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2381373002/#ps60001
(title: "BUILD.gn changes")

[commit-bot: I haz the power, 2016-10-10 19:30:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 20:26:31]

Description was changed from

==========
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=652899
==========

to

==========
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=652899
==========

[commit-bot: I haz the power, 2016-10-10 20:26:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-10 20:30:13]

Description was changed from

==========
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=652899
==========

to

==========
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=652899

Committed: https://crrev.com/22c4d40260078dfe23612368f2ae9519729062ce
Cr-Commit-Position: refs/heads/master@{#424222}
==========

[commit-bot: I haz the power, 2016-10-10 20:30:14]

Patchset 4 (id:??) landed as
https://crrev.com/22c4d40260078dfe23612368f2ae9519729062ce
Cr-Commit-Position: refs/heads/master@{#424222}