Handle invalid URLs in share extension.

Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259
Committed: https://crrev.com/1186149ec71795fd017d8fef851d2dd10c2a7b56
Cr-Commit-Position: refs/heads/master@{#441062}

[Olivier, 2016-12-30 15:49:39]

Description was changed from

==========
Handle invalid URLs in share extension.

BUG=677259
==========

to

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.
Another CL will handle local data (contextual menu and menu entry).

BUG=677259
==========

[Olivier, 2016-12-30 15:49:39]

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

[Olivier, 2016-12-30 15:57:47]

Description was changed from

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.
Another CL will handle local data (contextual menu and menu entry).

BUG=677259
==========

to

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

BUG=677259
==========

[gambard, 2017-01-02 09:11:41]

lgtm with comments

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (left):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:102: 2];
Why removing the /2?

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (right):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:68: // Display the normal
share view.
s/Display/Displays

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:71: // Display an alert to
report an error to the user.
s/Display/Displays

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/strings/ios_share_extension_strings.grd (right):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/strings/ios_share_extension_strings.grd:139: <message
name="IDS_IOS_ERROR_MESSAGE_SHARE_EXTENSION" desc="Share extension message to
telle the user the URL cannot be processed. The name of the application will be
put in place of APPLICATION_NAME">
s/telle/tell.

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/strings/ios_share_extension_strings.grd:140:
APPLICATION_NAME cannot handle this link.
Aren't we using placeholder for that kind of things?

[Olivier, 2017-01-02 10:39:59]

Description was changed from

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

BUG=677259
==========

to

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259
==========

[Olivier, 2017-01-02 10:40:12]

Patchset #3 (id:40001) has been deleted

[Olivier, 2017-01-02 10:40:40]

Thanks.
PTAL

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (left):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:102: 2];
On 2017/01/02 09:11:41, gambard wrote:
> Why removing the /2?

In view did load, the size of Shareview is not final.
It can change if text or image is added as this is asynchronous.
It was fine as animation started immediately, but now that we can have errors,
the widget could be visible at the bottom.

Changing the constraint to be closer to the actual intended behavior

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (right):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:68: // Display the normal
share view.
On 2017/01/02 09:11:41, gambard wrote:
> s/Display/Displays

Done.

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:71: // Display an alert to
report an error to the user.
On 2017/01/02 09:11:41, gambard wrote:
> s/Display/Displays

Done.

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/strings/ios_share_extension_strings.grd (right):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/strings/ios_share_extension_strings.grd:139: <message
name="IDS_IOS_ERROR_MESSAGE_SHARE_EXTENSION" desc="Share extension message to
telle the user the URL cannot be processed. The name of the application will be
put in place of APPLICATION_NAME">
On 2017/01/02 09:11:41, gambard wrote:
> s/telle/tell.

Done.

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/strings/ios_share_extension_strings.grd:140:
APPLICATION_NAME cannot handle this link.
On 2017/01/02 09:11:41, gambard wrote:
> Aren't we using placeholder for that kind of things?

placeholder handling is in ios/chrome/common which is not compiled in share
extension.
I would like to avoid including it.

[gambard, 2017-01-02 10:55:08]

Still lgtm with nits

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
File ios/chrome/share_extension/strings/ios_share_extension_strings.grd (right):

https://codereview.chromium.org/2608913002/diff/20001/ios/chrome/share_extens...
ios/chrome/share_extension/strings/ios_share_extension_strings.grd:140:
APPLICATION_NAME cannot handle this link.
On 2017/01/02 10:40:40, Olivier Robin wrote:
> On 2017/01/02 09:11:41, gambard wrote:
> > Aren't we using placeholder for that kind of things?
> 
> placeholder handling is in ios/chrome/common which is not compiled in share
> extension.
> I would like to avoid including it.

Acknowledged.

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (right):

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:37: // This constrains the
center of the widget to be vertically in the center
Why adding a "s" to constraint?

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:128:
[_widgetVerticalPlacementConstraint setActive:NO];
Nit: you could use dot syntax for readability, here and for the other
constraints

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:129:
_widgetVerticalPlacementConstraint = [[_shareView centerYAnchor]
Same for the centerYAnchor

[Olivier, 2017-01-02 12:23:31]

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

[Olivier, 2017-01-02 12:23:31]

+noyau as owner of ios/chrome/browser/share_extension

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
File ios/chrome/share_extension/share_view_controller.mm (right):

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:37: // This constrains the
center of the widget to be vertically in the center
On 2017/01/02 10:55:08, gambard wrote:
> Why adding a "s" to constraint?

Because constraint is a noun and constrain is a verb.

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:128:
[_widgetVerticalPlacementConstraint setActive:NO];
On 2017/01/02 10:55:08, gambard wrote:
> Nit: you could use dot syntax for readability, here and for the other
> constraints

setActive has a lot of consequences, so I prefer the call syntax.

https://codereview.chromium.org/2608913002/diff/60001/ios/chrome/share_extens...
ios/chrome/share_extension/share_view_controller.mm:129:
_widgetVerticalPlacementConstraint = [[_shareView centerYAnchor]
On 2017/01/02 10:55:08, gambard wrote:
> Same for the centerYAnchor

Done.

[noyau (Ping after 24h), 2017-01-02 12:51:28]

lgtm, but please add a second owner.

https://codereview.chromium.org/2608913002/diff/80001/ios/chrome/browser/shar...
File ios/chrome/browser/share_extension/OWNERS (right):

https://codereview.chromium.org/2608913002/diff/80001/ios/chrome/browser/shar...
ios/chrome/browser/share_extension/OWNERS:1: olivierrobin@chromium.org
Needs two owners, both chrome committers.

[Olivier, 2017-01-02 12:53:50]

https://codereview.chromium.org/2608913002/diff/80001/ios/chrome/browser/shar...
File ios/chrome/browser/share_extension/OWNERS (right):

https://codereview.chromium.org/2608913002/diff/80001/ios/chrome/browser/shar...
ios/chrome/browser/share_extension/OWNERS:1: olivierrobin@chromium.org
On 2017/01/02 12:51:28, noyau (OOO until Jan 2) wrote:
> Needs two owners, both chrome committers.

Done.

[Olivier, 2017-01-02 12:53:57]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-02 12:53:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
gambard@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2608913002/#ps100001
(title: "adding gambard as OWNER")

[commit-bot: I haz the power, 2017-01-02 12:54: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, 2017-01-02 13:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483361637223140,
"parent_rev": "081079d81fa3fbd3f324fded89041022fad87d7d", "commit_rev":
"d48796b16d2c99de5fd995bd7f76378546acc7b2"}

[commit-bot: I haz the power, 2017-01-02 13:05:02]

Description was changed from

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259
==========

to

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259

Review-Url: https://codereview.chromium.org/2608913002
==========

[commit-bot: I haz the power, 2017-01-02 13:05:02]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2017-01-02 15:55:24]

Description was changed from

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259

Review-Url: https://codereview.chromium.org/2608913002
==========

to

==========
Handle invalid URLs in share extension.

Show an error dialog is scheme is not HTTP or HTTPS.
Do not process item if scheme is not HTTP or HTTPS.

Adds olivierrobin@chromium.org as owner of
ios/chrome/browser/share_extension/

BUG=677259

Committed: https://crrev.com/1186149ec71795fd017d8fef851d2dd10c2a7b56
Cr-Commit-Position: refs/heads/master@{#441062}
==========

[commit-bot: I haz the power, 2017-01-02 15:55:25]

Patchset 5 (id:??) landed as
https://crrev.com/1186149ec71795fd017d8fef851d2dd10c2a7b56
Cr-Commit-Position: refs/heads/master@{#441062}