Stop using UIPasteboardChangedNotification.

Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592
Committed: https://crrev.com/fe97735842a1acb2c289538c40a14bc78164312e
Cr-Commit-Position: refs/heads/master@{#412788}

[jif, 2016-08-10 13:22:54]

Description was changed from

==========
Stop depending on UIPasteboardChangedNotification.

BUG=None
==========

to

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications on
pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed when we attempt
to display its content.

BUG=628592
==========

[jif, 2016-08-10 13:23:02]

Description was changed from

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications on
pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed when we attempt
to display its content.

BUG=628592
==========

to

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592
==========

[jif, 2016-08-10 13:23:18]

jif@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[jif, 2016-08-10 13:23:45]

I haven't updated the unittests yet, but can you still TAL?

[Olivier, 2016-08-10 13:46:42]

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
File components/open_from_clipboard/clipboard_recent_content_ios.h (right):

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.h:35: void
CheckIfPasteboardChanged();
Check... is not a name for a method that updates the state and does not return
anything. May be Update...IfNeeded?

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (right):

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.mm:87:
base::TimeDelta::FromSeconds(10);  // Hours(3);
don't forget to restore.

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.mm:143: if
(!url_from_pasteboard.is_valid()) {
remove !

[jif-google, 2016-08-11 13:37:52]

Patchset #2 (id:20001) has been deleted

[jif-google, 2016-08-11 13:39:54]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-08-11 13:39:55]

thanks. ptal.

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
File components/open_from_clipboard/clipboard_recent_content_ios.h (right):

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.h:35: void
CheckIfPasteboardChanged();
On 2016/08/10 13:46:41, Olivier Robin wrote:
> Check... is not a name for a method that updates the state and does not return
> anything. May be Update...IfNeeded?

Done. UpdatePasteboardIfNeeded sucked, so I simply went with UpdateIfNeeded.

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (right):

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.mm:87:
base::TimeDelta::FromSeconds(10);  // Hours(3);
On 2016/08/10 13:46:41, Olivier Robin wrote:
> don't forget to restore.

Acknowledged.

https://codereview.chromium.org/2230983002/diff/1/components/open_from_clipbo...
components/open_from_clipboard/clipboard_recent_content_ios.mm:143: if
(!url_from_pasteboard.is_valid()) {
On 2016/08/10 13:46:41, Olivier Robin wrote:
> remove !

Oh yes.

[jif-google, 2016-08-17 09:40:23]

ping

[Olivier, 2016-08-17 11:54:30]

Oups, did the comments, but did not send them
2 questions, then if answer is WAI, LGTM

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (left):

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_ios.mm:255: void
ClipboardRecentContentIOS::RecentURLDisplayed() {
not sure why this was needed and is not needed anymore.

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (right):

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_ios.mm:141:
base::RecordAction(base::UserMetricsAction("MobileOmniboxClipboardChanged"));
This is changing the semantic of the metric. Is this wanted?

[jif, 2016-08-17 15:43:01]

thanks.

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (left):

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_ios.mm:255: void
ClipboardRecentContentIOS::RecentURLDisplayed() {
On 2016/08/17 11:54:29, Olivier Robin wrote:
> not sure why this was needed and is not needed anymore.

On 2016/08/17 11:54:29, Olivier Robin wrote:
> not sure why this was needed and is not needed anymore.

added in:
https://codereview.chromium.org/1288733002
and then _not_ used in:
https://chromereviews.googleplex.com/227867013

The different patchset of 227867013 probably hold the answer, but I haven't
looked.

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
File components/open_from_clipboard/clipboard_recent_content_ios.mm (right):

https://codereview.chromium.org/2230983002/diff/40001/components/open_from_cl...
components/open_from_clipboard/clipboard_recent_content_ios.mm:141:
base::RecordAction(base::UserMetricsAction("MobileOmniboxClipboardChanged"));
On 2016/08/17 11:54:29, Olivier Robin wrote:
> This is changing the semantic of the metric. Is this wanted?

Indeed, it's changing the semantic in 2 ways:
1/ We are not listening to the notification, so we are missing some events.
2/ We are now recording _all_ clipboard changes, not just when the URL changes.

Point 1/ is not wanted, but inevitable.
Point 2/ is wanted: ClipboardRecentContent was never supposed to be just for
URLs, it was supposed to be extended to support images, regular text, etc...
This 
ClipboardRecentContent was not supposed to be only for URLs,

[jif, 2016-08-17 16:19:01]

(ptal)

[jif, 2016-08-17 16:19:42]

On 2016/08/17 16:19:01, jif wrote:
> (ptal)

nevermind, forgot that it LGTY if everything was WAI.

[jif, 2016-08-17 16:19:51]

The CQ bit was checked by jif@chromium.org

[commit-bot: I haz the power, 2016-08-17 16:20:10]

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-08-17 16:39:41]

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

[commit-bot: I haz the power, 2016-08-17 16:39:42]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jif-google, 2016-08-18 09:45:06]

The CQ bit was checked by jif@google.com

[jif-google, 2016-08-18 09:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2230983002/#ps60001
(title: "Fixed unittest.")

[commit-bot: I haz the power, 2016-08-18 09:45:23]

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-08-18 10:20:23]

Description was changed from

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592
==========

to

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592
==========

[commit-bot: I haz the power, 2016-08-18 10:20:24]

Committed patchset #3 (id:60001)

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

Description was changed from

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592
==========

to

==========
Stop using UIPasteboardChangedNotification.

iOS10 does not trigger UIPasteboardChangedNotification notifications
on pasteboard change ( filed bug with Apple: rdr://27477065 ).
This CL is a workaround where we check if the pasteboard changed
when we attempt to display its content.

BUG=628592

Committed: https://crrev.com/fe97735842a1acb2c289538c40a14bc78164312e
Cr-Commit-Position: refs/heads/master@{#412788}
==========

[commit-bot: I haz the power, 2016-08-18 10:22:31]

Patchset 3 (id:??) landed as
https://crrev.com/fe97735842a1acb2c289538c40a14bc78164312e
Cr-Commit-Position: refs/heads/master@{#412788}