[page-speed] r2558 committed - Add impact computation to AvoidPlugins (issue 1060001)

[page-...@googlecode.com]

Revision: 2558
Author: dbat...@google.com
Date: Thu Nov 14 23:49:00 2013 UTC
Log: Add impact computation to AvoidPlugins (issue 1060001)

http://code.google.com/p/page-speed/source/detail?r=2558

Modified:
/lib/trunk/src/pagespeed/proto/pagespeed_output.proto
/lib/trunk/src/pagespeed/rules/avoid_plugins.cc
/lib/trunk/src/pagespeed/rules/avoid_plugins_test.cc

=======================================
--- /lib/trunk/src/pagespeed/proto/pagespeed_output.proto Thu Nov 14
16:35:39 2013 UTC
+++ /lib/trunk/src/pagespeed/proto/pagespeed_output.proto Thu Nov 14
23:49:00 2013 UTC
@@ -366,6 +366,10 @@
optional int32 y = 5;
optional int32 width = 6;
optional int32 height = 7;
+
+ // How much of the above the fold viewport is covered by this plugin.
+ // 0 is none, 1.0 is the entirety of the viewport.
+ optional double atf_ratio = 11;
}

message ServerResponseTimeDetails {
=======================================
--- /lib/trunk/src/pagespeed/rules/avoid_plugins.cc Wed Nov 13 16:12:02
2013 UTC
+++ /lib/trunk/src/pagespeed/rules/avoid_plugins.cc Thu Nov 14 23:49:00
2013 UTC
@@ -106,6 +106,18 @@
{AvoidPluginsDetails_PluginType_JAVA, "Java"}
};

+// Base impact to be assigned to all plugins. Currently equal to
+// Rule::kImpactMediumCutoff
+static const double kPluginBaseImpact = 3.0;
+
+// If we have dimensions for a plugin, it's impact will be the
kPluginBaseImpact
+// plus the percentage of the ATF content covered by this plugin times this
+// constant. The logic behind the current value is if a single plugin is
more
+// than 20% of the ATF content, it should trigger a high impact result.
+// Therefore, it is currently equal to
+// (Rule::kImpactHighCutoff - Rule::kImpactMediumCutoff) / 0.2.
+static const double kPluginAtfImpactMultiplier = (10.0 - 3.0) / 0.2;
+
// Maximum depth to recurse when looking for a child of an object or
element
// tag that embeds a plugin in order to avoid double counting nested tags.
//
@@ -317,6 +329,26 @@
avoid_plugins_details->set_y(y);
avoid_plugins_details->set_width(width);
avoid_plugins_details->set_height(height);
+
+ const int viewport_w = rule_input_->pagespeed_input().viewport_width();
+ const int viewport_h =
rule_input_->pagespeed_input().viewport_height();
+
+ // Sanity check that the viewport width and height are set.
+ if (viewport_w > 0 && viewport_h > 0) {
+ const int atf_pixels = viewport_w * viewport_h;
+
+ const int clamped_x1 = std::min(std::max(x, 0), viewport_w);
+ const int clamped_y1 = std::min(std::max(y, 0), viewport_h);
+ const int clamped_x2 = std::min(std::max(x + width, 0), viewport_w);
+ const int clamped_y2 = std::min(std::max(y + height, 0), viewport_h);
+
+ const int atf_plugin_pixels =
+ (clamped_x2 - clamped_x1) * (clamped_y2 - clamped_y1);
+
+ avoid_plugins_details->set_atf_ratio(
+ static_cast<double>(atf_plugin_pixels) /
+ static_cast<double>(atf_pixels));
+ }
}
}

@@ -829,19 +861,20 @@

double AvoidPlugins::ComputeResultImpact(const InputInformation&
input_info,
const Result& result) {
- // TODO(dbathgate): We don't currently have a way to measure savings
from UX
- // rules. For now, return a fixed 1.0 impact if any plugins were
detected on
- // the page. This should probably take into account the number and size
of
- // the plugins.
- if (result.resource_urls_size() > 0) {
- return 1.0;
+ const ResultDetails& details = result.details();
+ if (details.HasExtension(AvoidPluginsDetails::message_set_extension)) {
+ const AvoidPluginsDetails& plugin_details = details.GetExtension(
+ AvoidPluginsDetails::message_set_extension);
+ if (plugin_details.has_atf_ratio()) {
+ return kPluginBaseImpact +
+ (kPluginAtfImpactMultiplier * plugin_details.atf_ratio());
+ }
}
- return 0;
+
+ return kPluginBaseImpact;
}

bool AvoidPlugins::IsExperimental() const {
- // TODO(dbathgate): Update ComputeResultImpact before graduating from
- // experimental.
return true;
}

=======================================
--- /lib/trunk/src/pagespeed/rules/avoid_plugins_test.cc Wed Nov 6
16:08:31 2013 UTC
+++ /lib/trunk/src/pagespeed/rules/avoid_plugins_test.cc Thu Nov 14
23:49:00 2013 UTC
@@ -20,6 +20,7 @@

using pagespeed::rules::AvoidPlugins;
using pagespeed::Resource;
+using pagespeed::Rule;
using pagespeed_testing::FakeDomDocument;
using pagespeed_testing::FakeDomElement;
using pagespeed_testing::PagespeedRuleTest;
@@ -113,18 +114,59 @@
embed->AddAttribute("type", kFlashMime);
embed->AddAttribute("src", "flash");
CheckOneUrl(kFlashBlock, "http://example.com/flash");
+ // No layout information, score as medium impact.
+ EXPECT_EQ(Rule::kImpactMediumCutoff, ComputeRuleImpact());
+}
+
+TEST_F(AvoidPluginsTest, FlashEmbed20PercentOfSize) {
+ SetViewportWidthAndHeight(100, 100); // 10000 px
+ FakeDomElement* embed = FakeDomElement::New(body(), "embed");
+ embed->AddAttribute("type", kFlashMime);
+ embed->AddAttribute("src", kSwfUrl);
+ embed->SetCoordinates(5, 5);
+ embed->SetActualWidthAndHeight(50, 40); // 2000 px
+ std::string expected =
+ std::string() + kSummary + kFlashBlock +
+ " " + kSwfUrl + " (50 x 40) final[5,5,50,40].\n";
+ CheckFormattedOutput(expected);
+ // One plugin that's 20% of the ATF should be a high impact result.
+ EXPECT_EQ(Rule::kImpactHighCutoff, ComputeRuleImpact());
+}
+
+TEST_F(AvoidPluginsTest, TwoClippedFlashEmbed20PercentOfSize) {
+ SetViewportWidthAndHeight(100, 100);
+ FakeDomElement* embed = FakeDomElement::New(body(), "embed");
+ embed->AddAttribute("type", kFlashMime);
+ embed->AddAttribute("src", kSwfUrl);
+ embed->SetCoordinates(0, 0);
+ embed->SetActualWidthAndHeight(50, 40);
+ FakeDomElement* embed_2 = FakeDomElement::New(body(), "embed");
+ embed_2->AddAttribute("type", kFlashMime);
+ embed_2->AddAttribute("src", kSwfUrl);
+ embed_2->SetCoordinates(100 - 50, 100 - 40);
+ embed_2->SetActualWidthAndHeight(800, 900);
+ std::string expected =
+ std::string() + kSummary + kFlashBlock +
+ " " + kSwfUrl + " (50 x 40) final[0,0,50,40].\n" +
+ " " + kSwfUrl + " (800 x 900) final[50,60,800,900].\n";
+ CheckFormattedOutput(expected);
+ // Each plugin is 20% of the ATF viewport after clipping.
+ EXPECT_EQ(2 * Rule::kImpactHighCutoff, ComputeRuleImpact());
}

TEST_F(AvoidPluginsTest, FlashEmbedSize) {
+ SetViewportWidthAndHeight(1024, 768);
FakeDomElement* embed = FakeDomElement::New(body(), "embed");
embed->AddAttribute("type", kFlashMime);
embed->AddAttribute("src", kSwfUrl);
- embed->SetCoordinates(111, 222);
- embed->SetActualWidthAndHeight(400, 800);
+ embed->SetCoordinates(11, 22);
+ embed->SetActualWidthAndHeight(400, 300);
std::string expected =
std::string() + kSummary + kFlashBlock +
- " " + kSwfUrl + " (400 x 800) final[111,222,400,800].\n";
+ " " + kSwfUrl + " (400 x 300) final[11,22,400,300].\n";
CheckFormattedOutput(expected);
+ EXPECT_NEAR(3.0 + ((10.0 - 3.0) / 0.2) * ((400.0*300.0) /
(1024.0*768.0)),
+ ComputeRuleImpact(), 0.01);
}

TEST_F(AvoidPluginsTest, FlashObjectSimple) {
@@ -160,7 +202,9 @@
" http://example.com/a.swf (400 x 800) final[111,222,400,800].\n" +
" http://example.com/b.swf\n";
CheckFormattedOutput(expected);
- EXPECT_EQ(2.0, ComputeRuleImpact());
+ // Since a viewport wasn't provided, this should be scored as two plugins
+ // with unknown area.
+ EXPECT_EQ(6.0, ComputeRuleImpact());
}

TEST_F(AvoidPluginsTest, FlashActiveXObject) {