Skip to content

Commit

Permalink
feedback: Add initial GetScreenshot mojom service
Browse files Browse the repository at this point in the history
- Take a screenshot of the primary display.
- Add GetScreenshot mojom service to retrieve the screenshot.

In order to share the screenshot, the user has to opt in. We have gotten
privacy approval [3].

The screenshot taking part is not finalized. Options are still being
explored and evaluated. One concern with the current implementation is
that it could capture part of the UI of the feedback tool because while
the screenshot is being taken, the UI is being created in parallel.

A better solution will be implemented in a followup CL.

Demo:
 - A screenshot thumbnail [1].
 - A larger image shown when hover [2].

1. https://screenshot.googleplex.com/AsTikVb9GMcqCq3.
2. https://screenshot.googleplex.com/9yM6bNw5ZLyzdvS.
3. https://bugs.chromium.org/p/chromium/issues/detail?id=1151153.

Bug: b:185624798
Test: browser_tests --gtest_filter=OSFeedbackBrowserTest.*; ash_webui_unittests --gtest_filter=FeedbackServiceProviderTest.*;
Change-Id: I0970db340f437524beccdb841f917cf80f0de82f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3630624
Commit-Queue: Xiangdong Kong <xiangdongkong@google.com>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Gavin Williams <gavinwill@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001646}
  • Loading branch information
xiangdong kong authored and Chromium LUCI CQ committed May 10, 2022
1 parent a8f74f2 commit edf00f9
Show file tree
Hide file tree
Showing 11 changed files with 155 additions and 4 deletions.
5 changes: 5 additions & 0 deletions ash/webui/os_feedback_ui/backend/feedback_service_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ void FeedbackServiceProvider::GetFeedbackContext(
std::move(callback).Run(std::move(feedback_context));
}

void FeedbackServiceProvider::GetScreenshotPng(
GetScreenshotPngCallback callback) {
feedback_delegate_->GetScreenshotPng(std::move(callback));
}

void FeedbackServiceProvider::SendReport(ReportPtr report,
SendReportCallback callback) {
feedback_delegate_->SendReport(std::move(report), std::move(callback));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class FeedbackServiceProvider

// os_feedback_ui::mojom::FeedbackServiceProvider:
void GetFeedbackContext(GetFeedbackContextCallback callback) override;
void GetScreenshotPng(GetScreenshotPngCallback callback) override;
void SendReport(os_feedback_ui::mojom::ReportPtr report,
SendReportCallback callback) override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ namespace {

constexpr char kPageUrl[] = "https://www.google.com/";
constexpr char kSignedInUserEmail[] = "test_user_email@test.com";
const std::vector<uint8_t> kFakePngData = {42, 22, 26, 13, 7, 16, 8, 2};

} // namespace

Expand All @@ -48,6 +49,10 @@ class TestOsFeedbackDelegate : public OsFeedbackDelegate {
return kSignedInUserEmail;
}

void GetScreenshotPng(GetScreenshotPngCallback callback) override {
std::move(callback).Run(kFakePngData);
}

void SendReport(os_feedback_ui::mojom::ReportPtr report,
SendReportCallback callback) override {
std::move(callback).Run(SendReportStatus::kSuccess);
Expand Down Expand Up @@ -75,6 +80,15 @@ class FeedbackServiceProviderTest : public testing::Test {
return out_feedback_context;
}

// Call the GetScreenshotPng of the remote provider async and return the
// response.
std::vector<uint8_t> GetScreenshotPngAndWait() {
std::vector<uint8_t> out_png_data;
FeedbackServiceProviderAsyncWaiter(provider_remote_.get())
.GetScreenshotPng(&out_png_data);
return out_png_data;
}

// Call the SendReport of the remote provider async and return the
// response.
SendReportStatus SendReportAndWait(ReportPtr report) {
Expand All @@ -99,6 +113,12 @@ TEST_F(FeedbackServiceProviderTest, GetFeedbackContext) {
EXPECT_EQ(kPageUrl, feedback_context->page_url.value().spec());
}

// Test that GetScreenshotPng returns a response with correct status.
TEST_F(FeedbackServiceProviderTest, GetScreenshotPng) {
auto png_data = GetScreenshotPngAndWait();
EXPECT_EQ(kFakePngData, png_data);
}

// Test that SendReport returns a response with correct status.
TEST_F(FeedbackServiceProviderTest, SendReportSuccess) {
ReportPtr report = Report::New();
Expand Down
5 changes: 5 additions & 0 deletions ash/webui/os_feedback_ui/backend/os_feedback_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class GURL;

namespace ash {

using GetScreenshotPngCallback =
base::OnceCallback<void(const std::vector<uint8_t>&)>;
using SendReportCallback =
base::OnceCallback<void(os_feedback_ui::mojom::SendReportStatus)>;

Expand All @@ -31,6 +33,9 @@ class OsFeedbackDelegate {
// Returns the normalized email address of the signed-in user associated with
// the browser context, if any.
virtual absl::optional<std::string> GetSignedInUserEmail() const = 0;
// Return the screenshot of the primary display in PNG format. It was taken
// right before the feedback tool is launched.
virtual void GetScreenshotPng(GetScreenshotPngCallback callback) = 0;
// Collect data and send the report to Google.
virtual void SendReport(os_feedback_ui::mojom::ReportPtr report,
SendReportCallback callback) = 0;
Expand Down
3 changes: 3 additions & 0 deletions ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ interface FeedbackServiceProvider {
// Returns the feedback context info, e.g., the email of the currently active
// or logged in user, the URL of the active page, etc.
GetFeedbackContext() => (FeedbackContext feedback_context);
// Returns the screenshot of the primary display in PNG format taken before
// the feedback tool is launched.
GetScreenshotPng() => (array<uint8> png_data);
// Sends a feedback report and returns its status.
SendReport(Report report) => (SendReportStatus status);
};
10 changes: 10 additions & 0 deletions ash/webui/os_feedback_ui/resources/fake_data.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,13 @@ export const fakeEmptyFeedbackContext = {
email: '',
pageUrl: {url: ''},
};

/** @type {!Array<number>} */
export const fakePngData = [
137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82,
0, 0, 0, 8, 0, 0, 0, 8, 8, 2, 0, 0, 0, 75, 109, 41,
220, 0, 0, 0, 34, 73, 68, 65, 84, 8, 215, 99, 120, 173, 168, 135,
21, 49, 0, 241, 255, 15, 90, 104, 8, 33, 129, 83, 7, 97, 163, 136,
214, 129, 93, 2, 43, 2, 0, 181, 31, 90, 179, 225, 252, 176, 37, 0,
0, 0, 0, 73, 69, 78, 68, 174, 66, 96, 130
];
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ export class FakeFeedbackServiceProvider {

// Setup method resolvers.
this.methods_.register('getFeedbackContext');
this.methods_.register('getScreenshotPng');
this.methods_.register('sendReport');
// Let sendReport return success by default.
this.methods_.setResult('sendReport', {status: SendReportStatus.kSuccess});
// Let getScreenshotPng return an empty array by default.
this.methods_.setResult('getScreenshotPng', {pngData: []});

/**
* Used to track how many times each method is being called.
Expand All @@ -30,6 +33,8 @@ export class FakeFeedbackServiceProvider {
/** @type {number} */
getFeedbackContext: 0,
/** @type {number} */
getScreenshotPng: 0,
/** @type {number} */
sendReport: 0,
};
}
Expand Down Expand Up @@ -69,6 +74,23 @@ export class FakeFeedbackServiceProvider {
return this.methods_.resolveMethod('sendReport');
}

/**
* @return {number}
*/
getScreenshotPngCallCount() {
return this.callCounts_.getScreenshotPng;
}

/**
* @return {!Promise<{
* pngData: !Array<!number>,
* }>}
*/
getScreenshotPng() {
this.callCounts_.getScreenshotPng++;
return this.methods_.resolveMethod('getScreenshotPng');
}

/**
* Sets the value that will be returned when calling getFeedbackContext().
* @param {!FeedbackContext} feedbackContext
Expand All @@ -77,11 +99,20 @@ export class FakeFeedbackServiceProvider {
this.methods_.setResult(
'getFeedbackContext', {feedbackContext: feedbackContext});
}

/**
* Sets the value that will be returned when calling sendReport().
* @param {!SendReportStatus} status
*/
setFakeSendFeedbackStatus(status) {
this.methods_.setResult('sendReport', {status: status});
}

/**
* Sets the value that will be returned when calling getScreenshotPng().
* @param {!Array<!number>} data
*/
setFakeScreenshotPng(data) {
this.methods_.setResult('getScreenshotPng', {pngData: data});
}
}
22 changes: 19 additions & 3 deletions ash/webui/os_feedback_ui/resources/feedback_flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,24 @@ export class FeedbackFlowElement extends PolymerElement {
});
}

/**
* @private
*/
fetchScreenshot_() {
const shareDataPage = this.shadowRoot.querySelector('share-data-page');
// Fetch screenshot if not fetched before.
if (!shareDataPage.screenshotUrl) {
this.feedbackServiceProvider_.getScreenshotPng().then((response) => {
if (response.pngData.length > 0) {
const blob = new Blob(
[Uint8Array.from(response.pngData)], {type: 'image/png'});
const imageUrl = URL.createObjectURL(blob);
shareDataPage.screenshotUrl = imageUrl;
}
});
}
}

/**
* @param {!Event} event
* @protected
Expand All @@ -89,9 +107,7 @@ export class FeedbackFlowElement extends PolymerElement {
case FeedbackFlowState.SEARCH:
this.currentState_ = FeedbackFlowState.SHARE_DATA;
this.description_ = event.detail.description;
// TODO(xiangdongkong): Fetch a real screenshot.
this.shadowRoot.querySelector('share-data-page').screenshotUrl =
'./app_icon_192.png';
this.fetchScreenshot_();
break;
case FeedbackFlowState.SHARE_DATA:
/** @type {!Report} */
Expand Down
41 changes: 41 additions & 0 deletions chrome/browser/ash/os_feedback/chrome_os_feedback_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@
#include <string>
#include <utility>

#include "ash/shell.h"
#include "ash/webui/os_feedback_ui/mojom/os_feedback_ui.mojom.h"
#include "base/bind.h"
#include "base/logging.h"
#include "base/memory/ref_counted.h"
#include "base/memory/ref_counted_memory.h"
#include "base/strings/utf_string_conversions.h"
#include "base/time/time.h"
#include "chrome/browser/browser_process.h"
Expand All @@ -30,6 +34,8 @@
#include "extensions/browser/api/feedback_private/feedback_service.h"
#include "net/base/network_change_notifier.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/aura/window.h"
#include "ui/snapshot/snapshot.h"
#include "url/gurl.h"

namespace ash {
Expand All @@ -41,6 +47,15 @@ feedback::FeedbackUploader* GetFeedbackUploaderForContext(
return feedback::FeedbackUploaderFactoryChrome::GetForBrowserContext(context);
}

void TakeScreenshot(
base::OnceCallback<void(scoped_refptr<base::RefCountedMemory>)> callback) {
aura::Window* primary_window = ash::Shell::GetPrimaryRootWindow();
if (primary_window) {
gfx::Rect rect = primary_window->bounds();
ui::GrabWindowSnapshotAsyncPNG(primary_window, rect, std::move(callback));
}
}

} // namespace

using ::ash::os_feedback_ui::mojom::SendReportStatus;
Expand All @@ -57,6 +72,10 @@ ChromeOsFeedbackDelegate::ChromeOsFeedbackDelegate(
Profile* profile,
scoped_refptr<extensions::FeedbackService> feedback_service)
: profile_(profile), feedback_service_(feedback_service) {
// TODO(xiangdongkong): Take screenshot first, then open the feedback app.
TakeScreenshot(base::BindOnce(&ChromeOsFeedbackDelegate::OnScreenshotTaken,
weak_ptr_factory_.GetWeakPtr()));

Browser* browser = BrowserList::GetInstance()->GetLastActive();
if (browser) {
// Save the last active page url before opening the feedback tool.
Expand Down Expand Up @@ -85,6 +104,19 @@ absl::optional<std::string> ChromeOsFeedbackDelegate::GetSignedInUserEmail()
.email;
}

void ChromeOsFeedbackDelegate::GetScreenshotPng(
GetScreenshotPngCallback callback) {
if (screenshot_png_data_ && screenshot_png_data_.get()) {
std::vector<uint8_t> data(
screenshot_png_data_->data(),
screenshot_png_data_->data() + screenshot_png_data_->size());
std::move(callback).Run(data);
} else {
std::vector<uint8_t> empty_data;
std::move(callback).Run(empty_data);
}
}

void ChromeOsFeedbackDelegate::SendReport(
os_feedback_ui::mojom::ReportPtr report,
SendReportCallback callback) {
Expand Down Expand Up @@ -123,4 +155,13 @@ void ChromeOsFeedbackDelegate::OnSendFeedbackDone(SendReportCallback callback,
std::move(callback).Run(send_status);
}

void ChromeOsFeedbackDelegate::OnScreenshotTaken(
scoped_refptr<base::RefCountedMemory> data) {
if (data && data.get()) {
screenshot_png_data_ = std::move(data);
} else {
LOG(ERROR) << "failed to take screenshot.";
}
}

} // namespace ash
8 changes: 8 additions & 0 deletions chrome/browser/ash/os_feedback/chrome_os_feedback_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@

#include "ash/webui/os_feedback_ui/backend/os_feedback_delegate.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "url/gurl.h"

class Profile;

namespace base {
class RefCountedMemory;
} // namespace base

namespace extensions {
class FeedbackService;
} // namespace extensions
Expand All @@ -37,17 +42,20 @@ class ChromeOsFeedbackDelegate : public OsFeedbackDelegate {
std::string GetApplicationLocale() override;
absl::optional<GURL> GetLastActivePageUrl() override;
absl::optional<std::string> GetSignedInUserEmail() const override;
void GetScreenshotPng(GetScreenshotPngCallback callback) override;
void SendReport(os_feedback_ui::mojom::ReportPtr report,
SendReportCallback callback) override;

private:
void OnSendFeedbackDone(SendReportCallback callback, bool status);
void OnScreenshotTaken(scoped_refptr<base::RefCountedMemory> data);

// TODO(xiangdongkong): make sure the profile_ cannot be destroyed while
// operations are pending.
raw_ptr<Profile> profile_;
scoped_refptr<extensions::FeedbackService> feedback_service_;
absl::optional<GURL> page_url_;
scoped_refptr<base::RefCountedMemory> screenshot_png_data_;

base::WeakPtrFactory<ChromeOsFeedbackDelegate> weak_ptr_factory_{this};
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import {fakeFeedbackContext, fakeSearchResponse} from 'chrome://os-feedback/fake_data.js';
import {fakeFeedbackContext, fakePngData, fakeSearchResponse} from 'chrome://os-feedback/fake_data.js';
import {FakeFeedbackServiceProvider} from 'chrome://os-feedback/fake_feedback_service_provider.js';
import {FakeHelpContentProvider} from 'chrome://os-feedback/fake_help_content_provider.js';
import {FeedbackFlowElement, FeedbackFlowState} from 'chrome://os-feedback/feedback_flow.js';
Expand Down Expand Up @@ -145,6 +145,8 @@ export function FeedbackFlowTestSuite() {
// Should stay on search page when click the continue button.
activePage = page.shadowRoot.querySelector('.iron-selected');
assertEquals('searchPage', activePage.id);
assertEquals(0, feedbackServiceProvider.getScreenshotPngCallCount());
feedbackServiceProvider.setFakeScreenshotPng(fakePngData);

const clickPromise = eventToPromise('continue-click', page);

Expand All @@ -164,6 +166,15 @@ export function FeedbackFlowTestSuite() {
// Should move to share data page when click the continue button.
activePage = page.shadowRoot.querySelector('.iron-selected');
assertEquals('shareDataPage', activePage.id);

// Verify that the getScreenshotPng is called once.
assertEquals(1, feedbackServiceProvider.getScreenshotPngCallCount());
const screenshotImg =
activePage.shadowRoot.querySelector('#screenshotImage');
assertTrue(!!screenshotImg);
assertTrue(!!screenshotImg.src);
// Verify that the src of the screenshot image is set.
assertTrue(screenshotImg.src.startsWith('blob:chrome://os-feedback/'));
});

// Test the navigation from share data page back to search page when click
Expand Down

0 comments on commit edf00f9

Please sign in to comment.