1455310 - macOS "Share" > "Reminders" missing a link to the current page

Reporter

Description

•

6 years ago

Right now when selecting the  "..." (page action) > "Share" > "Reminders" a new reminder overlay appears with no content. I quickly checked what Safari is doing with this feature and there the URL gets added to the reminder body, as well as a mini-screenshot with a nice animation. All that would be ideal, but at least the URL would be required to make the option worth while IMHO.

Dale Harvey (:daleharvey)

Assignee

Updated

•

6 years ago

Assignee: nobody → dharvey

Dale Harvey (:daleharvey)

Assignee

Comment 2

•

6 years ago

So the reminder service uses a completely different api to read its data into, found similiar code @ https://github.com/chromium/chromium/blob/master/chrome/browser/ui/cocoa/share_menu_controller.mm#L207 for chromium, take a look into adding it

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/239646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239646/

Dale Harvey (:daleharvey)

Assignee

Comment 4

•

6 years ago

Adds the activity that "Reminders" picks the data up from, also based on seeing a few more examples and the chrome implmentation did some cleaning up the code around, using package names should be less brittle than using the title (I didnt find them to be translated, but not certain they are never)

Markus Stange [:mstange]

Comment 5

•

6 years ago

mozreview-review

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

https://reviewboard.mozilla.org/r/239646/#review245442

::: widget/cocoa/nsMacSharingService.mm:108
(Diff revision 1)
> +                                initWithActivityType:NSUserActivityTypeBrowsingWeb];
> +    if ([pageUrl.scheme hasPrefix:@"http"]) {
> +      [activity setWebpageURL:pageUrl];
> +    }
> +    [activity setTitle:pageTitle];
> +    [activity becomeCurrent];

Do we need to call resignCurrent on this at some point? I wonder what the side-effects are of keeping this activity current forever. Or does the activity become non-current automatically if the NSUserActivity object is destroyed? This could probably be tested by adding the eligibleForHandoff property and seeing for how long the URL is available for handoff on other devices.

Attachment #8970877 - Flags: review?(mstange) → review+

Comment hidden (mozreview-request)

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239646/diff/1-2/

Dale Harvey (:daleharvey)

Assignee

Comment 7

•

6 years ago

So the activity will persist for the lifetime of the app as far as I could tell, so added a delegate that lets me invalidate the activity as soon as we have used it, update the mozreview and pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5817d239b5b252e89e22cf8baf5f50af0769e8, will give it a little time to comment then land later today

Pulsebot

Comment 8

•

6 years ago

Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48b66743b97b
Add title data and activity to sharing. r=mstange

Markus Stange [:mstange]

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

https://reviewboard.mozilla.org/r/239646/#review246142

shareActivity is alloc'd but never released, so I'm pretty sure it leaks.

I'd prefer a solution that does not require global variables or a delegate that is basically shutdown-leaked.

How about this:
 - Create a new delegate any time something is shared to the reminders service
 - Give the delagate a pointer to both the service and the shareActivity
 - After calling invalidate, make the delegate unregister itself from the service and release the shareActivity that's stored on it.
 - And during development, use some NSLog(@"...") statements in dealloc methods in order to confirm that things aren't leaked.

Sorry for the post-landing r-, but because of the leaks I think this is worth backing out.

Attachment #8970877 - Flags: review+ → review-

Cosmin Sabou [:CosminS]

Comment 10

•

6 years ago

Backed out changeset 48b66743b97b (bug 1455310) for xpcshell failures on widget/tests/unit/test_macsharingservice.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=48b66743b97b3ed2452a2d6a436f5cfeed3b48ae&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=xpcshell&selectedJob=175976430

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175975682&repo=autoland&lineNumber=7721

Backout link: https://hg.mozilla.org/integration/autoland/rev/31104079416b1f4a0db7cb08505000314fa8062f

09:03:19     INFO -  TEST-START | widget/tests/unit/test_macsharingservice.js
09:03:19  WARNING -  TEST-UNEXPECTED-FAIL | widget/tests/unit/test_macsharingservice.js | xpcshell return code: 0
09:03:19     INFO -  TEST-INFO took 375ms
09:03:19     INFO -  >>>>>>>
09:03:19     INFO -  PID 7141 | JavaScript strict warning: resource://gre/modules/XPCOMUtils.jsm, line 277: ReferenceError: reference to undefined property 1
09:03:19     INFO -  PID 7141 | [7141, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2678
09:03:19     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
09:03:19     INFO -  TEST-PASS | widget/tests/unit/test_macsharingservice.js | test_getSharingProviders - [test_getSharingProviders : 16] There are providers returned - true == true
09:03:19  WARNING -  TEST-UNEXPECTED-FAIL | widget/tests/unit/test_macsharingservice.js | test_getSharingProviders/< - [test_getSharingProviders/< : 18] Provider has title - false == true
09:03:19     INFO -  /Users/cltbld/tasks/task_1524835552/build/tests/xpcshell/tests/widget/tests/unit/test_macsharingservice.js:test_getSharingProviders/<:18
09:03:19     INFO -  /Users/cltbld/tasks/task_1524835552/build/tests/xpcshell/tests/widget/tests/unit/test_macsharingservice.js:test_getSharingProviders:17
09:03:19     INFO -  /Users/cltbld/tasks/task_1524835552/build/tests/xpcshell/tests/widget/tests/unit/test_macsharingservice.js:run_test:28
09:03:19     INFO -  /Users/cltbld/tasks/task_1524835552/build/tests/xpcshell/head.js:_execute_test:515
09:03:19     INFO -  -e:null:1
09:03:19     INFO -  exiting test
09:03:19     INFO -  PID 7141 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
09:03:19     INFO -  PID 7141 | [7141, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 343
09:03:19     INFO -  PID 7141 | [7141, Main Thread] WARNING: '!gThread', file /builds/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp, line 399
09:03:19     INFO -  PID 7141 | [7141, Main Thread] WARNING: OOPDeinit() without successful OOPInit(): file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 3332
09:03:19     INFO -  PID 7141 | nsStringStats
09:03:19     INFO -  PID 7141 |  => mAllocCount:           3000
09:03:19     INFO -  PID 7141 |  => mReallocCount:          214
09:03:19     INFO -  PID 7141 |  => mFreeCount:            3000
09:03:19     INFO -  PID 7141 |  => mShareCount:           1777
09:03:19     INFO -  PID 7141 |  => mAdoptCount:            143
09:03:19     INFO -  PID 7141 |  => mAdoptFreeCount:        143
09:03:19     INFO -  PID 7141 |  => Process ID: 7141, Thread ID: 140735262651136
09:03:19     INFO -  <<<<<<<

Flags: needinfo?(dharvey)

Dale Harvey (:daleharvey)

Assignee

Comment 11

•

6 years ago

Apologies for the leak + test failure, completely didnt notice the orange X on the try run, as for the leak I assumed that the cocoa file was loaded once at startup so we just had a global delegate that was always active

Trying to do as suggested, however hitting a snag, if I try to cleanup the shareService, remove its delegate or release it firefox crashes hard, code is @ https://reviewboard.mozilla.org/r/239644/diff/4#index_header with relevant part posted here

@implementation SharingServiceDelegate
- (void)cleanup {
  NSLog(@"shareService cleanup");

  [self.shareActivity resignCurrent];
  [self.shareActivity invalidate];
  [self.shareActivity release];

  // TODO: Either of the below calls crash firefox
  //[self.shareService setDelegate:nil];
  //[self.shareService release];

Flags: needinfo?(dharvey)

Dale Harvey (:daleharvey)

Assignee

Comment 12

•

6 years ago

Marcus, hey do you have any ideas about ^? cant seem to get this delegate to dealloc properly. Thank you

Flags: needinfo?(mstange)

Comment hidden (mozreview-request)

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239646/diff/2-3/

Marco Bonardo [:mak]

Updated

•

6 years ago

Priority: -- → P1

Whiteboard: [fxsearch]

Markus Stange [:mstange]

Comment 14

•

6 years ago

I'm hoping to take a look at this today.

Markus Stange [:mstange]

Comment 15

•

6 years ago

Attached patch fixup patch — Details — Splinter Review

This patch seems to make things work correctly. I realized that we don't have to detach the delegate from the sharing service. As long as the share activity gets destroyed at the right time, and as long as it's clear that the service owns the delegate (and is responsible for destroying it), everything should work correctly. And it looks like the sharing service automatically detaches the delegate from it once sharing has completed.

I also sprinkled some more retains and releases over the code. Now every alloc and every retain is mirrored with a release in the appropriate place.

Flags: needinfo?(mstange)

Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]

Updated

•

6 years ago

Flags: qe-verify+

Dale Harvey (:daleharvey)

Assignee

Comment 16

•

6 years ago

oh ok, thanks for this, yeh I was planning to switch the delegate to a modified constructor when the deallc was fixed, it looks like the [shareDelegate release] was the bit I was missing, thanks a lot.

Marco Bonardo [:mak]

Updated

•

6 years ago

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239646/diff/3-4/

Dale Harvey (:daleharvey)

Assignee

Comment 18

•

6 years ago

Updated with the patch, in testing I can see the delegate being dealloc'd, fixed test and pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a2cd245a9dd8db5b899119fa499805c3a67c63b

Markus Stange [:mstange]

Comment 19

•

6 years ago

mozreview-review

Comment on attachment 8970877 [details]
Bug 1455310 - Add title data and activity to sharing.

https://reviewboard.mozilla.org/r/239646/#review247726

Attachment #8970877 - Flags: review?(mstange) → review+

Pulsebot

Comment 20

•

6 years ago

Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1039c7e00471
Add title data and activity to sharing. r=mstange

Dale Harvey (:daleharvey)

Assignee

Comment 21

•

6 years ago

All the tests passed when retried and non of the intermittents looked anywhere near this code. Thanks for the help Marcus and apologies for landing prematurely, will be more patient next time. Cheers

Andreea Pavel [:apavel]

Comment 22

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/1039c7e00471

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox61: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 61

Daniel Bodea [:danibodea]

Comment 23

•

6 years ago

Hi Dave,

I have attempted to validate this issue's fix on OS X 12.6 and 13.2. As I am not sure about the specifications of this feature I have to ask if the following flow is intended:

1. When the user clicks the browser's "other options" button (three dots displayed in a vertical position), then "Share", then "Reminders", a pop-up will be displayed. The pages tab name is displayed in the reminder's name field and the domain of the page (not he full link) is displayed in the reminder's description field. 
The problem I see here is that only the page's domain gets displayed instead of the whole link.

2. When the user reaches the Reminders app and clicks the domain link saved, the redirect will be to the main page of the site (only domain link), instead of the correct page.

But,

3. As a workaround, when the user reaches the Reminders app, he can click on the default set browser's icon displayed at the right of the reminder, he WILL be redirected correctly to the initially saved page (not only domain link).

Is this the intended design?

Flags: needinfo?(dharvey)

Dale Harvey (:daleharvey)

Assignee

Comment 24

•

6 years ago

> Is this the intended design?

This is all the behaviour of the Reminders application on MacOS, trying out the same in Safari has the same behaviour and I dont think there is anything we can do to change how Reminders handles incoming links, so yup it is all expected

Flags: needinfo?(dharvey)

Daniel Bodea [:danibodea]

Comment 25

•

6 years ago

Based on the comment above, I am setting this issue as VERIFIED.

Status: RESOLVED → VERIFIED

Daniel Bodea [:danibodea]

Updated

•

6 years ago

status-firefox61: fixed → verified

Adrian Florinescu [:aflorinescu]

Updated

•

3 years ago

Bug 1455310 - Add title data and activity to sharing. 6 years ago Dale Harvey (:daleharvey) 59 bytes, text/x-review-board-request	mstange : review+	Details
fixup patch 6 years ago Markus Stange [:mstange] 3.66 KB, patch		Details \| Diff \| Splinter Review