Expose thumbnails of pages to iOS share extensions.

Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A
thumbnail of the page should be shown.
BUG=686049
Review-Url: https://codereview.chromium.org/2645653003
Cr-Commit-Position: refs/heads/master@{#447977}
Committed: https://chromium.googlesource.com/chromium/src/+/4a8cf9449a9df503e1993433249a9e6923b5d810

[jif-google, 2017-01-20 10:03:58]

Description was changed from

==========
Foo.

BUG=None.
==========

to

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=None.
==========

[jif-google, 2017-01-20 10:03:58]

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

[jif, 2017-01-20 10:04:12]

Description was changed from

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=None.
==========

to

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

[jif, 2017-01-20 10:04:27]

ptal

[jif, 2017-01-20 10:04:39]

Description was changed from

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

to

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

[jif, 2017-01-20 10:05:00]

Description was changed from

==========
Offer thumbnails to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

to

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

[Olivier, 2017-01-20 10:14:00]

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
This activity is the standard one. I don't think it should be named after the
password management usage .

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h:34: // is
used by extensions displaying thumbnails of what is shared, for example
It is not really used by extension, right?
It is used by Chrome when extension request a thumbnail.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
I would add block to the name.
I wondered until I reached this point of the CL if a * was missing.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
const CGSize& ?

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:19:
ThumbnailGenerator thumbNailGeneratorForTab(Tab* tab);
ThumbnailGeneratorForTab

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:16:
__weak Tab* weakTab = tab;
This is C++. Use weak_tab

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:18:
Tab* strongTab = weakTab;
nit: Is weaktab needed?
Will self not be retained on call on next line?

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:31: BOOL
isPagePrintable = [tab viewForPrinting] != nil;
C++ case

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:87: #import
"ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h"
alphabetical order

[Olivier, 2017-01-20 10:14:01]

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
This activity is the standard one. I don't think it should be named after the
password management usage .

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h:34: // is
used by extensions displaying thumbnails of what is shared, for example
It is not really used by extension, right?
It is used by Chrome when extension request a thumbnail.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
I would add block to the name.
I wondered until I reached this point of the CL if a * was missing.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
const CGSize& ?

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:19:
ThumbnailGenerator thumbNailGeneratorForTab(Tab* tab);
ThumbnailGeneratorForTab

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:16:
__weak Tab* weakTab = tab;
This is C++. Use weak_tab

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:18:
Tab* strongTab = weakTab;
nit: Is weaktab needed?
Will self not be retained on call on next line?

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:31: BOOL
isPagePrintable = [tab viewForPrinting] != nil;
C++ case

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:87: #import
"ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h"
alphabetical order

[jif-google, 2017-01-20 15:14:23]

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

[pkl (ping after 24h if needed), 2017-01-20 18:30:30]

pkl@chromium.org changed reviewers:
+ pkl@chromium.org
- pkl@google.com

[pkl (ping after 24h if needed), 2017-01-20 18:30:31]

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
On 2017/01/20 10:14:00, Olivier Robin wrote:
> This activity is the standard one. I don't think it should be named after the
> password management usage .

If your proposal is to rename UIActivity{FindLoginAction}Source to something
else, I'm OK with that. Do you have a name in mind?

If you are thinking of replacing this with a different type or changing the
implementation of the class UIActivityFindLoginActionSource, I'd advise you to
think further. The implementation of the method
-activityViewController:dataTypeIdentifierForActivityType: is critical for
Password Managers to function correctly.

[Olivier, 2017-01-20 19:29:25]

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
On 2017/01/20 18:30:31, pklpkl wrote:
> On 2017/01/20 10:14:00, Olivier Robin wrote:
> > This activity is the standard one. I don't think it should be named after
the
> > password management usage .
> 
> If your proposal is to rename UIActivity{FindLoginAction}Source to something
> else, I'm OK with that. Do you have a name in mind?
> 
> If you are thinking of replacing this with a different type or changing the
> implementation of the class UIActivityFindLoginActionSource, I'd advise you to
> think further. The implementation of the method
> -activityViewController:dataTypeIdentifierForActivityType: is critical for
> Password Managers to function correctly.

My suggestion was to rename the variable and/or the class and/or change the
order the actions are descibed in comment (the main usage of this action being
social media, and then password managers).

URLProvider would be more similar to textProvider and imageProvider below?

[Olivier, 2017-01-20 20:41:59]

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/chrome_activity_item_source.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_source.mm:202:
return _thumbnailGenerator(size);
What happens if the block is nil?
(it may work... I don't really know).

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:20:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
Is the size used?

[jif, 2017-01-24 09:59:56]

Patchset #1 (id:1) has been deleted

[jif, 2017-01-24 10:00:20]

Patchset #1 (id:20001) has been deleted

[jif-google, 2017-01-24 10:28:50]

Patchset #2 (id:60001) has been deleted

[jif-google, 2017-01-24 10:28:59]

Patchset #2 (id:80001) has been deleted

[jif, 2017-01-24 10:29:50]

ptal

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
On 2017/01/20 10:14:00, Olivier Robin wrote:
> This activity is the standard one. I don't think it should be named after the
> password management usage .

I agree.
+pkl for a second opinion.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:200:
[[UIActivityFindLoginActionSource alloc]
On 2017/01/20 19:29:25, Olivier Robin wrote:
> On 2017/01/20 18:30:31, pklpkl wrote:
> > On 2017/01/20 10:14:00, Olivier Robin wrote:
> > > This activity is the standard one. I don't think it should be named after
> the
> > > password management usage .
> > 
> > If your proposal is to rename UIActivity{FindLoginAction}Source to something
> > else, I'm OK with that. Do you have a name in mind?
> > 
> > If you are thinking of replacing this with a different type or changing the
> > implementation of the class UIActivityFindLoginActionSource, I'd advise you
to
> > think further. The implementation of the method
> > -activityViewController:dataTypeIdentifierForActivityType: is critical for
> > Password Managers to function correctly.
> 
> My suggestion was to rename the variable and/or the class and/or change the
> order the actions are descibed in comment (the main usage of this action being
> social media, and then password managers).
> 
> URLProvider would be more similar to textProvider and imageProvider below?

Yes, I think URLProvider would be a better name. But maybe in a separate CL.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_source.h:34: // is
used by extensions displaying thumbnails of what is shared, for example
On 2017/01/20 10:14:00, Olivier Robin wrote:
> It is not really used by extension, right?
> It is used by Chrome when extension request a thumbnail.

Done.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/chrome_activity_item_source.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_source.mm:202:
return _thumbnailGenerator(size);
On 2017/01/20 20:41:59, Olivier Robin wrote:
> What happens if the block is nil?
> (it may work... I don't really know).

_thumbnailGenerator can't be nil (see DCHECK in initializer, but don't see
comment on initializer...).

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
On 2017/01/20 10:14:00, Olivier Robin wrote:
> I would add block to the name.
> I wondered until I reached this point of the CL if a * was missing.

Done.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:14:
typedef UIImage* (^ThumbnailGenerator)(CGSize);
On 2017/01/20 10:14:00, Olivier Robin wrote:
> const CGSize& ?

Done.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h:19:
ThumbnailGenerator thumbNailGeneratorForTab(Tab* tab);
On 2017/01/20 10:14:00, Olivier Robin wrote:
> ThumbnailGeneratorForTab

Done.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:16:
__weak Tab* weakTab = tab;
On 2017/01/20 10:14:00, Olivier Robin wrote:
> This is C++. Use weak_tab

But this is Obj-C++.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:18:
Tab* strongTab = weakTab;
On 2017/01/20 10:14:00, Olivier Robin wrote:
> nit: Is weaktab needed?
> Will self not be retained on call on next line?

I don't understand, there is no self here.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:20:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
On 2017/01/20 20:41:59, Olivier Robin wrote:
> Is the size used?

Done.

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/share_to_data_builder.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/share_to_data_builder.mm:31: BOOL
isPagePrintable = [tab viewForPrinting] != nil;
On 2017/01/20 10:14:00, Olivier Robin wrote:
> C++ case

It's an obj-c++ function with no c++ functionality used (except the namespace
outside).

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2645653003/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:87: #import
"ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.h"
On 2017/01/20 10:14:00, Olivier Robin wrote:
> alphabetical order

Done.

[Olivier, 2017-01-24 16:54:05]

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:22:
if (strongTab.browserState->IsOffTheRecord())
browser state is a C++ object.
If strongTab is nil, browserState will be null and you get a null dereference
exception.

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:22:
if (strongTab.browserState->IsOffTheRecord())
nit: I don't think tab can change between incognito and not incognito.
Can you test this before and not give a ThumbnailGeneratorBlock for incognito
tabs?

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/share_to_data.mm (right):

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:83: CGSize size =
CGSizeMake(40, 40);
Add comment.
Block are equal if they give the same thumbnail.

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:88: BOOL
thumbnailDataIsEqual = thumbnailData == otherThumbnailData ||
I don't think I see anywhere in tests where this can be false (you always have a
single snapshot in tests).
Is this ever effectively used?

[jif, 2017-01-25 15:17:29]

Thanks. Changed a bunch of things. PTAL

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:22:
if (strongTab.browserState->IsOffTheRecord())
On 2017/01/24 16:54:05, Olivier Robin wrote:
> browser state is a C++ object.
> If strongTab is nil, browserState will be null and you get a null dereference
> exception.

oops

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:22:
if (strongTab.browserState->IsOffTheRecord())
On 2017/01/24 16:54:05, Olivier Robin wrote:
> nit: I don't think tab can change between incognito and not incognito.
> Can you test this before and not give a ThumbnailGeneratorBlock for incognito
> tabs?

Ack.
Because I don't like returning nil blocks, I'll return a block that does
nothing.

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/share_to_data.mm (right):

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:83: CGSize size =
CGSizeMake(40, 40);
On 2017/01/24 16:54:05, Olivier Robin wrote:
> Add comment.
> Block are equal if they give the same thumbnail.

Ack.
I moved the code which makes this comment obsolete I think.

https://codereview.chromium.org/2645653003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:88: BOOL
thumbnailDataIsEqual = thumbnailData == otherThumbnailData ||
On 2017/01/24 16:54:05, Olivier Robin wrote:
> I don't think I see anywhere in tests where this can be false (you always have
a
> single snapshot in tests).
> Is this ever effectively used?

Done.
The bitmap comparison is used in the bvc_unittest.mm.

[jif, 2017-01-25 15:24:40]

Description was changed from

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.

BUG=682315
==========

to

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

BUG=682315
==========

[jif, 2017-01-25 15:26:26]

Patchset #3 (id:120001) has been deleted

[Olivier, 2017-01-25 16:10:33]

https://codereview.chromium.org/2645653003/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:199:
UIActivityFindLoginActionSource* loginActionProvider =
Still need a name change.

https://codereview.chromium.org/2645653003/diff/140001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/140001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:19: bool UIImagesAreEqual(UIImage*
image1, UIImage* image2);
You may use |UIImage isEqual:| instead.

[jif, 2017-01-25 16:44:29]

jif@chromium.org changed reviewers:
+ marq@chromium.org, sdefresne@chromium.org
- pkl@chromium.org

[jif, 2017-01-25 16:45:43]

Thank you!

Updating reviewers list:
-pkl because the renaming will be done in a separate CL
+marq, OWNERS of ios/chrome/browser/ui
+sdefresne, OWNERS of ui/base/test/uis/OWNERS

https://codereview.chromium.org/2645653003/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/activity_service_controller.mm
(right):

https://codereview.chromium.org/2645653003/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/activity_service_controller.mm:199:
UIActivityFindLoginActionSource* loginActionProvider =
On 2017/01/25 16:10:33, Olivier Robin wrote:
> Still need a name change.

Added a TODO.

https://codereview.chromium.org/2645653003/diff/140001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/140001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:19: bool UIImagesAreEqual(UIImage*
image1, UIImage* image2);
On 2017/01/25 16:10:33, Olivier Robin wrote:
> You may use |UIImage isEqual:| instead.

My understanding is that isEqual compares the pointer backing the UIImage.
Those would be identical if two UIImages were loaded from the same ressource.

[Olivier, 2017-01-25 17:26:00]

lgtm

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:18: UIImage* imageWithSolidColor =
UIGraphicsGetImageFromCurrentImageContext();
I think you should use _ case in this file.

[Olivier, 2017-01-25 17:35:23]

On 2017/01/25 17:26:00, Olivier Robin wrote:
> lgtm
> 
>
https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
> File ui/base/test/ios/ui_image_test_utils.mm (right):
> 
>
https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
> ui/base/test/ios/ui_image_test_utils.mm:18: UIImage* imageWithSolidColor =
> UIGraphicsGetImageFromCurrentImageContext();
> I think you should use _ case in this file.

FYI: I filed rdar://30188145 about the UIImage isEqual "bug".

[sdefresne, 2017-01-26 14:50:41]

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:25: }  // test
// namespace test

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:26: DCHECK(image1.CGImage);
Should you check the images dimension? Looks like you only look at the image
data and length, but if you have a MxN and a NxM image with a solid color, your
function returns true when I would expect it to return false.

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:31: CFDataRef dataRef1 =
I think you should use ScopedCFTypeRef to avoid forgetting the CFRelease.

  base::ScopedCFTypeRef<CFDataRef> dataRef1(
      CGDataProviderCopyData(CGImageGetDataProvider(image1.CGImage));
  ...

[marq (ping after 24h), 2017-01-26 16:28:10]

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:29:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
Nit: You can just call this method on weakTab here; nil objects return nil, and
ResizeImage(nil, ...) should also be nil.

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/share_to_data.mm (right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:60: self.title = title;
You shouldn't use setter methods in an initializer:
https://google.github.io/styleguide/objcguide.xml?showone=Avoid_Accessors_Dur...

[jif, 2017-01-27 10:54:47]

Patchset #5 (id:180001) has been deleted

[jif, 2017-01-27 10:58:39]

Patchset #5 (id:200001) has been deleted

[jif, 2017-01-27 11:56:12]

thank you all.

ptal.

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:29:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
On 2017/01/26 16:28:09, marq wrote:
> Nit: You can just call this method on weakTab here; nil objects return nil,
and
> ResizeImage(nil, ...) should also be nil.

The pattern of calling a method on a weak object is dangerous because the object
could released on a separate thread.

In this particular case, the tab can only be released on the UI-thread, but
that's a detail this function shouldn't know about.

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/activity_services/share_to_data.mm (right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/share_to_data.mm:60: self.title = title;
On 2017/01/26 16:28:09, marq wrote:
> You shouldn't use setter methods in an initializer:
>
https://google.github.io/styleguide/objcguide.xml?showone=Avoid_Accessors_Dur...

Done.

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:25: }  // test
On 2017/01/26 14:50:40, sdefresne wrote:
> // namespace test

Done.

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:18: UIImage* imageWithSolidColor =
UIGraphicsGetImageFromCurrentImageContext();
On 2017/01/25 17:26:00, Olivier Robin wrote:
> I think you should use _ case in this file.

Done.

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:26: DCHECK(image1.CGImage);
On 2017/01/26 14:50:40, sdefresne wrote:
> Should you check the images dimension? Looks like you only look at the image
> data and length, but if you have a MxN and a NxM image with a solid color,
your
> function returns true when I would expect it to return false.

Haha, nice catch!
Done.

https://codereview.chromium.org/2645653003/diff/160001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:31: CFDataRef dataRef1 =
On 2017/01/26 14:50:40, sdefresne wrote:
> I think you should use ScopedCFTypeRef to avoid forgetting the CFRelease.
> 
>   base::ScopedCFTypeRef<CFDataRef> dataRef1(
>       CGDataProviderCopyData(CGImageGetDataProvider(image1.CGImage));
>   ...
> 

Thank you!

[jif, 2017-01-27 11:56:26]

The CQ bit was checked by jif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 11:56:42]

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

[marq (ping after 24h), 2017-01-27 12:52:56]

LGTM with nits.

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:29:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
On 2017/01/27 11:56:11, jif wrote:
> On 2017/01/26 16:28:09, marq wrote:
> > Nit: You can just call this method on weakTab here; nil objects return nil,
> and
> > ResizeImage(nil, ...) should also be nil.
> 
> The pattern of calling a method on a weak object is dangerous because the
object
> could released on a separate thread.
> 
> In this particular case, the tab can only be released on the UI-thread, but
> that's a detail this function shouldn't know about.

You are of course correct.

You can (and should), however, remove the nil check on |strongTab| --
https://google.github.io/styleguide/objcguide.xml?showone=nil_Checks#nil_Checks.

https://codereview.chromium.org/2645653003/diff/220001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/220001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:7: #include "base/logging.h"
I don't see where this is used.

[commit-bot: I haz the power, 2017-01-27 12:55:39]

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

[commit-bot: I haz the power, 2017-01-27 12:55:40]

Dry run: This issue passed the CQ dry run.

[jif, 2017-01-27 13:07:58]

Description was changed from

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

BUG=682315
==========

to

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A thumbnail of
the page should be shown.
BUG=686049
==========

[jif, 2017-01-27 13:09:25]

Description was changed from

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A thumbnail of
the page should be shown.
BUG=686049
==========

to

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A
thumbnail of the page should be shown.
BUG=686049
==========

[jif, 2017-01-27 13:51:55]

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm
(right):

https://codereview.chromium.org/2645653003/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/activity_services/chrome_activity_item_thumbnail_generator.mm:29:
[strongTab generateSnapshotWithOverlay:NO visibleFrameOnly:YES];
On 2017/01/27 12:52:56, marq wrote:
> On 2017/01/27 11:56:11, jif wrote:
> > On 2017/01/26 16:28:09, marq wrote:
> > > Nit: You can just call this method on weakTab here; nil objects return
nil,
> > and
> > > ResizeImage(nil, ...) should also be nil.
> > 
> > The pattern of calling a method on a weak object is dangerous because the
> object
> > could released on a separate thread.
> > 
> > In this particular case, the tab can only be released on the UI-thread, but
> > that's a detail this function shouldn't know about.
> 
> You are of course correct.
> 
> You can (and should), however, remove the nil check on |strongTab| --
>
https://google.github.io/styleguide/objcguide.xml?showone=nil_Checks#nil_Checks.

Done.

https://codereview.chromium.org/2645653003/diff/220001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/220001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:7: #include "base/logging.h"
On 2017/01/27 12:52:56, marq wrote:
> I don't see where this is used.

Good catch. Done.

[sdefresne, 2017-01-30 16:56:20]

lgtm

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:21: // Returns whether the pixels in
|image1| are equal to the pixels in |image2|.
Either use image1 in method too or use image_1 in comment :-)

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:38: if (length_1 == length_2) {
Can you use early return here too?

  if (length_1 != length_2)
    return false;

  const UInt8* ptr_1 = CFDataGetBytePtr(data_ref_1);
  const UInt8* ptr_2 = CFDataGetBytePtr(data_ref_2);
  return memcmp(ptr_1, ptr_2, length_1) == 0;

[jif, 2017-02-03 10:54:49]

Thank you

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.h (right):

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.h:21: // Returns whether the pixels in
|image1| are equal to the pixels in |image2|.
On 2017/01/30 16:56:20, sdefresne wrote:
> Either use image1 in method too or use image_1 in comment :-)

Done.

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
File ui/base/test/ios/ui_image_test_utils.mm (right):

https://codereview.chromium.org/2645653003/diff/240001/ui/base/test/ios/ui_im...
ui/base/test/ios/ui_image_test_utils.mm:38: if (length_1 == length_2) {
On 2017/01/30 16:56:20, sdefresne wrote:
> Can you use early return here too?
> 
>   if (length_1 != length_2)
>     return false;
> 
>   const UInt8* ptr_1 = CFDataGetBytePtr(data_ref_1);
>   const UInt8* ptr_2 = CFDataGetBytePtr(data_ref_2);
>   return memcmp(ptr_1, ptr_2, length_1) == 0;

Done.

[jif, 2017-02-03 10:55:33]

The CQ bit was checked by jif@chromium.org

[jif, 2017-02-03 10:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org, marq@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2645653003/#ps260001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2017-02-03 10:55:48]

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, 2017-02-03 12:05:28]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486119333112720,
"parent_rev": "d21a8899b64b77e5d759b3b0f894df9bdcdf5575", "commit_rev":
"4a8cf9449a9df503e1993433249a9e6923b5d810"}

[commit-bot: I haz the power, 2017-02-03 12:06:01]

Description was changed from

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A
thumbnail of the page should be shown.
BUG=686049
==========

to

==========
Expose thumbnails of pages to iOS share extensions.

This CLs implements UIActivityItemSource's
|activityViewController:thumbnailImageForActivityType:suggestedSize:|.
Also:
-factor out of the BVC the creation of ShareToData.
-remove unused UIActivityURLSource.
-fix ui/base/test/ios/OWNERS (broken by
https://codereview.chromium.org/1885043012 )

TEST=Share a page from Chrome iOS to the Chrome iOS extension. A
thumbnail of the page should be shown.
BUG=686049

Review-Url: https://codereview.chromium.org/2645653003
Cr-Commit-Position: refs/heads/master@{#447977}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a8cf9449a9df503e1993433249a...
==========

[commit-bot: I haz the power, 2017-02-03 12:06:02]

Committed patchset #7 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/4a8cf9449a9df503e1993433249a...